DevTools: Make sure UISC content is up-to-date when running performSearchInContent()

DevTools: Make sure UISC content is up-to-date when running performSearchInContent()

R=vsevik@chromium.org, vsevik
BUG=357614
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170559

[vsevik, 2014-03-28 16:18:36]

https://chromiumcodereview.appspot.com/216183003/diff/1/Source/devtools/front...
File Source/devtools/front_end/SourcesSearchScope.js (right):

https://chromiumcodereview.appspot.com/216183003/diff/1/Source/devtools/front...
Source/devtools/front_end/SourcesSearchScope.js:139: matchingFiles.put(file);
Can we use matchingFiles.put.bind(matchingFiles) instead?

https://chromiumcodereview.appspot.com/216183003/diff/1/Source/devtools/front...
Source/devtools/front_end/SourcesSearchScope.js:145: * @param
{!WebInspector.UISourceCode} sourceCode
s/sourceCode/uiSourceCode

https://chromiumcodereview.appspot.com/216183003/diff/1/Source/devtools/front...
File Source/devtools/front_end/UISourceCode.js (right):

https://chromiumcodereview.appspot.com/216183003/diff/1/Source/devtools/front...
Source/devtools/front_end/UISourceCode.js:265: * @param {function(boolean)=}
callback
Looks like this parameter is never used, why do you add it?

https://chromiumcodereview.appspot.com/216183003/diff/1/Source/devtools/front...
Source/devtools/front_end/UISourceCode.js:270: if (this._checkingContent) {
Why did you swap the conditionals?

[apavlov, 2014-03-30 13:02:08]

https://codereview.chromium.org/216183003/diff/1/Source/devtools/front_end/So...
File Source/devtools/front_end/SourcesSearchScope.js (right):

https://codereview.chromium.org/216183003/diff/1/Source/devtools/front_end/So...
Source/devtools/front_end/SourcesSearchScope.js:139: matchingFiles.put(file);
On 2014/03/28 16:18:36, vsevik wrote:
> Can we use matchingFiles.put.bind(matchingFiles) instead?

Done.

https://codereview.chromium.org/216183003/diff/1/Source/devtools/front_end/So...
Source/devtools/front_end/SourcesSearchScope.js:145: * @param
{!WebInspector.UISourceCode} sourceCode
On 2014/03/28 16:18:36, vsevik wrote:
> s/sourceCode/uiSourceCode

Saw sourceCode somewhere

https://codereview.chromium.org/216183003/diff/1/Source/devtools/front_end/UI...
File Source/devtools/front_end/UISourceCode.js (right):

https://codereview.chromium.org/216183003/diff/1/Source/devtools/front_end/UI...
Source/devtools/front_end/UISourceCode.js:265: * @param {function(boolean)=}
callback
On 2014/03/28 16:18:36, vsevik wrote:
> Looks like this parameter is never used, why do you add it?

Previously, not all paths would invoke the callback, now all of them do, but the
the this._checkingContent case the result is not actually valid. Fine with
dropping it for now.

https://codereview.chromium.org/216183003/diff/1/Source/devtools/front_end/UI...
Source/devtools/front_end/UISourceCode.js:270: if (this._checkingContent) {
On 2014/03/28 16:18:36, vsevik wrote:
> Why did you swap the conditionals?

This happened after a few steps of refactoring :)

[apavlov, 2014-03-31 16:23:09]

PTAL

[pfeldman, 2014-03-31 19:20:50]

https://codereview.chromium.org/216183003/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/SourcesSearchScope.js (right):

https://codereview.chromium.org/216183003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/SourcesSearchScope.js:138:
files.forEach(matchingFiles.put.bind(matchingFiles));
While you are here, could you rename StringSet's put into add? Also, sounds like
StringSet.fromArray would make my eyes hurt less.

https://codereview.chromium.org/216183003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/SourcesSearchScope.js:140:
project.uiSourceCodes().filter(dirtySourceCodeFilter).forEach(appendFilePath);
my 8 lines against your 20

var uiSourceCodes = project.uiSourceCodes();
for (var i = 0; i < uiSourceCodes.length; ++i) {
    if (!uiSourceCodes[i].isDirty())
        continue;
    var path = uiSourceCodes[i].path();
    if (!matchingFiles.contains(path))
        files.push(path);
}

https://codereview.chromium.org/216183003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/SourcesSearchScope.js:186:
uiSourceCode.requestContent(contentLoaded.bind(this, uiSourceCode.path()));
... and when do we start using promises once again? :)

[apavlov, 2014-04-01 07:26:00]

https://codereview.chromium.org/216183003/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/SourcesSearchScope.js (right):

https://codereview.chromium.org/216183003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/SourcesSearchScope.js:138:
files.forEach(matchingFiles.put.bind(matchingFiles));
On 2014/03/31 19:20:51, pfeldman wrote:
> While you are here, could you rename StringSet's put into add? Also, sounds
like
> StringSet.fromArray would make my eyes hurt less.

Done.

https://codereview.chromium.org/216183003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/SourcesSearchScope.js:140:
project.uiSourceCodes().filter(dirtySourceCodeFilter).forEach(appendFilePath);
On 2014/03/31 19:20:51, pfeldman wrote:
> my 8 lines against your 20
> 
> var uiSourceCodes = project.uiSourceCodes();
> for (var i = 0; i < uiSourceCodes.length; ++i) {
>     if (!uiSourceCodes[i].isDirty())
>         continue;
>     var path = uiSourceCodes[i].path();
>     if (!matchingFiles.contains(path))
>         files.push(path);
> }

Accepted!

https://codereview.chromium.org/216183003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/SourcesSearchScope.js:186:
uiSourceCode.requestContent(contentLoaded.bind(this, uiSourceCode.path()));
On 2014/03/31 19:20:51, pfeldman wrote:
> ... and when do we start using promises once again? :)

Uhm... We've never used them... and will not, I promise! :)

[vsevik, 2014-04-01 09:10:20]

lgtm, but consider fixing one of the callback() calls below.

https://codereview.chromium.org/216183003/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/UISourceCode.js (right):

https://codereview.chromium.org/216183003/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/UISourceCode.js:275: callback();
This one is actually incorrect, I don't think we should call back before content
reloading initiated by another call of this method is finished

[apavlov, 2014-04-01 09:12:24]

https://codereview.chromium.org/216183003/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/UISourceCode.js (right):

https://codereview.chromium.org/216183003/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/UISourceCode.js:275: callback();
On 2014/04/01 09:10:20, vsevik wrote:
> This one is actually incorrect, I don't think we should call back before
content
> reloading initiated by another call of this method is finished

That's why I initially had a boolean callback argument :) My point was that we
should callback regardless of the check outcome, otherwise we can just get the
DevTools to freeze. Should I introduce a pending callbacks array instead?

[vsevik, 2014-04-01 09:49:34]

On 2014/04/01 09:12:24, apavlov wrote:
>
https://codereview.chromium.org/216183003/diff/60001/Source/devtools/front_en...
> File Source/devtools/front_end/UISourceCode.js (right):
> 
>
https://codereview.chromium.org/216183003/diff/60001/Source/devtools/front_en...
> Source/devtools/front_end/UISourceCode.js:275: callback();
> On 2014/04/01 09:10:20, vsevik wrote:
> > This one is actually incorrect, I don't think we should call back before
> content
> > reloading initiated by another call of this method is finished
> 
> That's why I initially had a boolean callback argument :) My point was that we
> should callback regardless of the check outcome, otherwise we can just get the
> DevTools to freeze. Should I introduce a pending callbacks array instead?

Yes, I think so.

[apavlov, 2014-04-01 11:14:32]

On 2014/04/01 09:49:34, vsevik wrote:
> On 2014/04/01 09:12:24, apavlov wrote:
> >
>
https://codereview.chromium.org/216183003/diff/60001/Source/devtools/front_en...
> > File Source/devtools/front_end/UISourceCode.js (right):
> > 
> >
>
https://codereview.chromium.org/216183003/diff/60001/Source/devtools/front_en...
> > Source/devtools/front_end/UISourceCode.js:275: callback();
> > On 2014/04/01 09:10:20, vsevik wrote:
> > > This one is actually incorrect, I don't think we should call back before
> > content
> > > reloading initiated by another call of this method is finished
> > 
> > That's why I initially had a boolean callback argument :) My point was that
we
> > should callback regardless of the check outcome, otherwise we can just get
the
> > DevTools to freeze. Should I introduce a pending callbacks array instead?
> 
> Yes, I think so.

Done.

[apavlov, 2014-04-01 11:24:20]

The CQ bit was checked by apavlov@chromium.org

[commit-bot: I haz the power, 2014-04-01 11:24:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apavlov@chromium.org/216183003/80001

[commit-bot: I haz the power, 2014-04-01 11:36:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-01 11:36:28]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[apavlov, 2014-04-01 13:10:01]

Committed patchset #4 manually as r170559 (presubmit successful).