Bug 40422 – Web Inspector: Port performSearch from InjectedScript to InspectorDOMAgent.

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

[Pavel Feldman]

Two reasons here:
1) We want to search within iframes from other domains and JS can't do that
2) We want to slowly get rid of InjectedScript

I am working on the port.

[Pavel Feldman]

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

[WebKit Review Bot]

Attachment 58404 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/inspector/InspectorDOMAgent.cpp:88:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/inspector/InspectorDOMAgent.cpp:103:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/inspector/InspectorDOMAgent.cpp:114:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/inspector/InspectorDOMAgent.cpp:125:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/inspector/InspectorDOMAgent.cpp:141:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/inspector/InspectorDOMAgent.cpp:616:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 6 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

Created attachment 58405 [details]
[PATCH] Same with proper formatting.

[WebKit Review Bot]

Attachment 58404 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3205155

[Pavel Feldman]

Created attachment 58408 [details]
[PATCH] Same + missing import for Chromium.

[WebKit Review Bot]

Attachment 58405 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3187144

[WebKit Review Bot]

Attachment 58408 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3226172

[Pavel Feldman]

Created attachment 58449 [details]
[PATCH] More imports! (is this going to end?).

[Yury Semikhatsky]

Comment on attachment 58449 [details]
[PATCH] More imports! (is this going to end?).


WebCore/inspector/InspectorResource.cpp: 
 +          jsonObject.set("suggestedFilename", m_suggestedFilename);
Remove suggestedFilename handling from inspecor.js too.

WebCore/inspector/InspectorDOMAgent.cpp:1554
 +      m_pendingMatchJobs.removeFirst();
Use takeFirst instead.

WebCore/inspector/InspectorDOMAgent.cpp:644
 +      RefPtr<NodeList> frameOwners = mainFrameDocument()->querySelectorAll("iframe, frame, object", ec);
Why not use FrameTree to iterate all frames in the page?

WebCore/inspector/InspectorDOMAgent.cpp:696
 +          m_pendingMatchJobs.append(new MatchPlainTextJob(*it, escapedQuery));
Please put this line first in the loop body and remove it from all the ifs above.

WebCore/inspector/InspectorDOMAgent.cpp:674
 +              m_pendingMatchJobs.append(new MatchXPathJob(*it, "//*[contains(name(), '" + escapedTagNameQuery + "')]"));
This could be implemented as

//*[contains(name(), 'escapedTagNameQuery') and string-length(substring-after(name(), 'escapedTagNameQuery')) = 0]


WebCore/inspector/InspectorDOMAgent.h:78
 +      class MatchJob {
Can you leave only forward declaration of the class here and put its definition into InspectorDOMAgent.cpp?

[Pavel Feldman]

(In reply to comment #9)
> (From update of attachment 58449 [details])
> 
> WebCore/inspector/InspectorResource.cpp: 
>  +          jsonObject.set("suggestedFilename", m_suggestedFilename);
> Remove suggestedFilename handling from inspecor.js too.
> 

I am just removing the dupe entry.

> WebCore/inspector/InspectorDOMAgent.cpp:1554
>  +      m_pendingMatchJobs.removeFirst();
> Use takeFirst instead.
>

Done. thanks.
 
> WebCore/inspector/InspectorDOMAgent.cpp:644
>  +      RefPtr<NodeList> frameOwners = mainFrameDocument()->querySelectorAll("iframe, frame, object", ec);
> Why not use FrameTree to iterate all frames in the page?
> 

I don't want to get outside DOM domain, documents' frame might be unavailable :)

> WebCore/inspector/InspectorDOMAgent.cpp:696
>  +          m_pendingMatchJobs.append(new MatchPlainTextJob(*it, escapedQuery));
> Please put this line first in the loop body and remove it from all the ifs above.
> 

I can't. We keep specific order of matches (ordered by relevance).

> WebCore/inspector/InspectorDOMAgent.cpp:674
>  +              m_pendingMatchJobs.append(new MatchXPathJob(*it, "//*[contains(name(), '" + escapedTagNameQuery + "')]"));
> This could be implemented as
> 
> //*[contains(name(), 'escapedTagNameQuery') and string-length(substring-after(name(), 'escapedTagNameQuery')) = 0]
> 

I am only porting code, don't want to fix it yet.

> 
> WebCore/inspector/InspectorDOMAgent.h:78
>  +      class MatchJob {
> Can you leave only forward declaration of the class here and put its definition into InspectorDOMAgent.cpp?

Done. thanks.

[Pavel Feldman]

Created attachment 58456 [details]
[PATCH] Review comments addressed.

[Pavel Feldman]

Created attachment 58464 [details]
[PATCH] With nice frame tree traversal.

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/inspector/elements-panel-search.html
	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/InspectorResource.cpp
	M	WebCore/inspector/front-end/ElementsPanel.js
	M	WebCore/inspector/front-end/InjectedScript.js
	M	WebCore/inspector/front-end/InjectedScriptAccess.js
	M	WebCore/inspector/front-end/InspectorBackendStub.js
	M	WebKit/chromium/ChangeLog
	M	WebKit/chromium/src/js/InspectorControllerImpl.js
Committed r61012

[Joseph Pecoraro]

Comment on attachment 58464 [details]
[PATCH] With nice frame tree traversal.

Good solution! Its nice to see that using clean OO that this
actually made the code more readable than JavaScript. =)

I know this was landed, and it looks good to me, I just have
some late comments (as usual).


> (WebCore::InspectorBackend::searchCanceled):
> (WebCore::InspectorDOMAgent::searchCanceled):
> (WebInspector.ElementsPanel.prototype.searchCanceled):

> +void InspectorDOMAgent::searchCanceled()
> +{
> +    if (m_matchJobsTimer.isActive())
> +        m_matchJobsTimer.stop();
> +    deleteAllValues(m_pendingMatchJobs);
> +    m_pendingMatchJobs.clear();
> +    m_searchResults.clear();
> +}

I know "searchCanceled" was the original name, but I think it
makes more sense to call this "cancelSearch". Because that
is what it is doing and that is what most of its callers want
to do.


> +    void performSearch(const String& query, bool runSynchronously);

It would be nice to have a comment here that runSynchronously is here
only for testing. There is a comment in the implementation, but it
might have been more noticeable here.


> WebCore/inspector/InspectorDOMAgent.cpp
> + 701         m_pendingMatchJobs.append(new MatchExactTagNamesJob(document, tagNameQuery));
> + 702         m_pendingMatchJobs.append(new MatchQuerySelectorAllJob(document, "[" + attributeNameQuery + "]"));
> + 703         m_pendingMatchJobs.append(new MatchQuerySelectorAllJob(document, whitespaceTrimmedQuery));

Can we remove line 701 in favor of the more generic line 703?
I always thought that:

  var str = '...';
  document.getElementsByTagName(str) ⊆ document.querySelectorAll(str)

I realize your changes here keep the results the same as the
JavaScript. My proposal would be to make this just:

  m_pendingMatchJobs.append(new MatchQuerySelectorAllJob(document, whitespaceTrimmedQuery));
  m_pendingMatchJobs.append(new MatchQuerySelectorAllJob(document, "[" + attributeNameQuery + "]"));


> WebCore/inspector/InspectorDOMAgent.h
> +#include "Document.h"
>  #include "EventListener.h"
>  #include "EventTarget.h"
>  #include "InspectorCSSStore.h"
> +#include "NodeList.h"
>  #include "ScriptArray.h"
>  #include "ScriptObject.h"
>  #include "ScriptState.h"
> +#include "Timer.h"
> ...
> -    class Document;

With the recent concern over build times, I've been taking
a look at these. NodeList is only used in the .cpp and not the
.h. Is there an advantage to putting this in the .h instead of
just the .cpp. Same thing for Document? Prolly not a big deal
in this case, but I'm interested in why this changed.


> +    m_matchJobsTimer.startOneShot(0.025);

Magic value. It was magic before too =(


> WebCore/inspector/front-end/InjectedScript.js
>  // Called from within InspectorController on the 'inspected page' side.
>  InjectedScript.reset = function()
>  {
> -    InjectedScript._searchResults = [];
> -    InjectedScript._includedInSearchResultsPropertyName = "__includedInInspectorSearchResults";
>  }
>  
>  InjectedScript.reset();

Can you just remove all of this code, including the lines
you left in? This looks the only caller, and it looks like
now it is just saving a reference (and calling) an empty
function. You now clear the search results in
InspectorDOMAgent::reset and InspectorDOMAgent::performSearch.

[Pavel Feldman]

Joe, all good suggestions, thanks.

> I know "searchCanceled" was the original name, but I think it
> makes more sense to call this "cancelSearch". Because that
> is what it is doing and that is what most of its callers want
> to do.
> 

True, I was porting the code and wanted to keep the original naming in the pure port change.

> 
> > +    void performSearch(const String& query, bool runSynchronously);
> 
> It would be nice to have a comment here that runSynchronously is here
> only for testing. There is a comment in the implementation, but it
> might have been more noticeable here.
> 

He-he, request to add comment into WebKit source :). In fact, I can see us using it not only for testing at some time. Imagine that alternate front-ends connect to inspector using the protocol, they might want to search synchronously.

> Can we remove line 701 in favor of the more generic line 703?
> I always thought that:
> 

This is a pure port and I did not want regressions. What you suggest may show tag name matches after the attribute matches, hence breaking the original order.

> > WebCore/inspector/InspectorDOMAgent.h
> > +#include "Document.h"
> >  #include "EventListener.h"
> >  #include "EventTarget.h"
> >  #include "InspectorCSSStore.h"
> > +#include "NodeList.h"
> >  #include "ScriptArray.h"
> >  #include "ScriptObject.h"
> >  #include "ScriptState.h"
> > +#include "Timer.h"
> > ...
> > -    class Document;
> 
> With the recent concern over build times, I've been taking
> a look at these. NodeList is only used in the .cpp and not the
> .h. Is there an advantage to putting this in the .h instead of
> just the .cpp. Same thing for Document? Prolly not a big deal
> in this case, but I'm interested in why this changed.
> 

Ok, this is my bad. First version of the patch had MatchJob declared in .h, so it needed imports. Then, driven with the build time concern, it moved to .cpp, but I forgot to wipe out the imports. Will fix in subsequent patch.

> 
> > +    m_matchJobsTimer.startOneShot(0.025);
> 
> Magic value. It was magic before too =(
> 

Magic indeed.

> 
> > WebCore/inspector/front-end/InjectedScript.js
> >  // Called from within InspectorController on the 'inspected page' side.
> >  InjectedScript.reset = function()
> >  {
> > -    InjectedScript._searchResults = [];
> > -    InjectedScript._includedInSearchResultsPropertyName = "__includedInInspectorSearchResults";
> >  }
> >  
> >  InjectedScript.reset();
> 
> Can you just remove all of this code, including the lines
> you left in? This looks the only caller, and it looks like
> now it is just saving a reference (and calling) an empty
> function. You now clear the search results in
> InspectorDOMAgent::reset and InspectorDOMAgent::performSearch.

My plan is to remove the whole thing if we can. Rationale: 1) injecting JS into the page might be harmful 2) it makes our remote debugging protocol not as clean as pure InspectorFrontend / InspectorBackend. I was hesitant removing this reset since it might have been overriden in the chromium port. Now I see it is not, so probably worth a cleanup next time we port something from it.

[Joseph Pecoraro]

> > It would be nice to have a comment here that runSynchronously
> > is here only for testing.
> 
> [...] they might want to search synchronously.

Sounds good.


> > Can we remove line 701 in favor of the more generic line 703?
> > I always thought that:
> 
> This is a pure port and I did not want regressions. What you suggest
> may show tag name matches after the attribute matches, hence
> breaking the original order.

Yes, I was aware, but I think this would still be for the better. It gets rid
of a full DOM tree traversal. I don't think this is a major issue, the same
results are returned, just their order is different (and hopefully faster).