[webkit-reviews] review granted: [Bug 40733] Web Inspector: move InjectedScript's get/setOuterHTML, addInspectedNode and clearConsole to natives. : [Attachment 58929] [PATCH] Same + IDL cleanup. Sorry for noise.

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 40733: Web Inspector: move InjectedScript's get/setOuterHTML,
addInspectedNode and clearConsole to natives.
https://bugs.webkit.org/show_bug.cgi?id=40733

Attachment 58929: [PATCH] Same + IDL cleanup. Sorry for noise.
https://bugs.webkit.org/attachment.cgi?id=58929&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
Some nice generic cleanup along the way, excellent!


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog

Could use a description about why this is desired. In the
past this was desired so information in <iframe>s could
be accessed easier. Is that the same case here?


> diff --git a/WebCore/inspector/InspectorDOMAgent.cpp
b/WebCore/inspector/InspectorDOMAgent.cpp
> -void InspectorDOMAgent::changeTagName(long callId, long nodeId, const
AtomicString& tagName, bool expanded)
> +void InspectorDOMAgent::changeTagName(long callId, long nodeId, const
String& tagName)
>  {
>      Node* oldNode = nodeForId(nodeId);
>      if (!oldNode || !oldNode->isElementNode()) {
>	   // Use -1 to denote an error condition.
> -	   m_frontend->didChangeTagName(callId, -1);
> +	   m_frontend->didChangeTagName(callId, 0);
>	   return;
>      }
>  
> +    bool childrenRequested = m_childrenRequested.contains(nodeId);

Good changes to this function (thanks for cleaning up after me)!

Does removing `expanded`, and switching to `m_childrenRequested`
handle cases where it was expanded previously but was collapsed
when edited. The scenario I'm thinking of:

  HTML:
  <div>
    <p>Hello World</p>
  </div>

  User Actions:
    - expand the <div> (m_childrenRequested gets populated)
    - collapse the <div> (does m_childrenRequested get altered?)
    - user edits <div> tagname to be <section>
    - is <section> collapsed or expanded? I would expect collapsed.

Also, please remove the now defunct comment:

  // Use -1 to denote an error condition.


> +	   Vector<long> m_inspectedNodes;

Is there a way to optimize this to be of a particular length?
We know there should only ever be 5 we want to keep, and 1
that should be deleted soon after. In the constructor, can
we limit this to a size of 6 or so. That way it doesn't take
up any more space than necessary?


> +++ b/WebCore/inspector/front-end/InjectedScript.js
> @@ -83,7 +83,6 @@ InjectedScript.releaseWrapperObjectGroup =
function(objectGroupName) {
>  // Called from within InspectorController on the 'inspected page' side.
>  InjectedScript.reset = function()
>  {
> -    InjectedScript._inspectedNodes = [];
>  }
>  
>  InjectedScript.reset();

Again, this is back to empty and can be removed, unless
you have a particular reason to keep it in. You mentioned
before that chromium might have overridden it, but it
turned out it wasn't overridden.


> InjectedScriptHost.clearConsoleMessages()
> InspectorBackend.clearConsoleMessages()

There are now two, identical functions on both sides. Maybe
there are others. Can you think of a way these can be shared,
so there aren't multiple implementations / duplicated code?
Thats out of the scope of this patch, but something to keep
in mind.


r=me unless the "expand" scenario I mentioned above doesn't work.