Bug 40733 – Web Inspector: move InjectedScript's get/setOuterHTML, addInspectedNode and clearConsole to natives.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Feldman]

Patch to follow.

[Pavel Feldman]

Created attachment 58928 [details]
[PATCH] Proposed change.

I am currently manually testing the change.

[Pavel Feldman]

Created attachment 58929 [details]
[PATCH] Same + IDL cleanup. Sorry for noise.

[Joseph Pecoraro]

Comment on attachment 58929 [details]
[PATCH] Same + IDL cleanup. Sorry for noise.

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.

[Pavel Feldman]

> Some nice generic cleanup along the way, excellent!

Stop appreciating and join the cleanup rush already :)

> 
> 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?
> 

Correct. + more rationale added. Done.

> > +    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.
> 

wasExpanded flag is still being used in the front-end, so that expand state would be restored. I am using additional information about the fact whether children were sent to the front-end from within DOM agent instead of passing this 'view' information in the request to the model. Knowing that children were requested is enough to provide desired behavior.

> Also, please remove the now defunct comment:
> 
>   // Use -1 to denote an error condition.
> 

Done here and in removeNode. Also assigned ExceptionCode code = 0, and renamed all 'code' to 'ec' for consistency.
> 
> > +        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?
> 

I would not bother. This happens when selecting node manually, i.e. user-initiated UI operation. Shifting an array of 5 is not a big deal.

> 
> > +++ 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.
> 

Ok, ok. Done.

> 
> > 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.
> 

Both are API facades immediately delegating to the same InspectorController's implementation. We need it to be both: a part of InspectorBackend API and for Console API's clear command (i.e. on InjectedScriptHost API).

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

I'll manually test prior to landing!

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/InjectedScriptHost.cpp
	M	WebCore/inspector/InjectedScriptHost.h
	M	WebCore/inspector/InjectedScriptHost.idl
	M	WebCore/inspector/InspectorBackend.cpp
	M	WebCore/inspector/InspectorBackend.h
	M	WebCore/inspector/InspectorBackend.idl
	M	WebCore/inspector/InspectorDOMAgent.cpp
	M	WebCore/inspector/InspectorDOMAgent.h
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/InspectorFrontend.h
	M	WebCore/inspector/front-end/ConsoleView.js
	M	WebCore/inspector/front-end/DOMAgent.js
	M	WebCore/inspector/front-end/ElementsPanel.js
	M	WebCore/inspector/front-end/ElementsTreeOutline.js
	M	WebCore/inspector/front-end/InjectedScript.js
	M	WebCore/inspector/front-end/InjectedScriptAccess.js
Committed r61317

[Joseph Pecoraro]

> Both are API facades immediately delegating to the same InspectorController's
> implementation. We need it to be both: a part of InspectorBackend API and for
> Console API's clear command (i.e. on InjectedScriptHost API).

Yes, we need it in both. But it is still duplicated code, no matter how simple
it is. A possible solution, but lame, would be some shared IDL object, like
InspectorSharedObject which contained the clearConsole. But, like I said,
that is a lame idea. Another idea might be an abstract InspectorShared class
that both InspectorBackend and InjectedScriptHost subclass. But that seems
like unnecessary complication and still both IDLs would have the duplicated method.

[Pavel Feldman]

> Yes, we need it in both. But it is still duplicated code, no matter how simple
> it is. A possible solution, but lame, would be some shared IDL object, like
> InspectorSharedObject which contained the clearConsole. But, like I said,
> that is a lame idea. Another idea might be an abstract InspectorShared class
> that both InspectorBackend and InjectedScriptHost subclass. But that seems
> like unnecessary complication and still both IDLs would have the duplicated method.

For the sake of the record, the fragment you are considering 'duplicated code' is:

>> BEGIN OF DUPLICATED CODE

if (m_inspectorController)
    m_inspectorController->clearConsoleMessages();

>> END OF DUPLICATED CODE

So you are suggesting organizing hierarchies of IDLs... Or classes. Where one of them defines remote debugging interface and the other serves  as a bridge to the injected javascript code (i.e. they are from different domains)... With InspectorBackend to stop being an actual binding shortly... Joe, I'd say I can live with the duplicated code above.