Remove some UI->FILE->UI thread hops in ExecuteCodeFunction

Remove some UI->FILE->UI thread hops in ExecuteCodeFunction.

There are 3 types of tasks in ExecuteCodeFunction that require FILE thread:
a) Reading the file content (for non-component extension files)
b) Retrieving the file GURL of the script file from base::FilePath
c) content l10n (if the script is CSS)

This CL does all of these in on UI->FILE hop and doesn't require
additional FILE thread hop.
Previously, the most common case, i.e. chrome.tabs.executeScript()
with {file:...}, would require UI->FILE->UI->FILE->UI. With this CL
this become UI->FILE->UI.

This CL accomplishes this with adding an optional callback to FileReader,
to specify additional tasks to be run on FILE thread.

So the thread hop changes are:
Component extension's executeScript() with file:... url would stay same.
*All* other means of executing script file:... url, e.g.
chrome.tabs.executeScript(,{file:..}) would require two (UI->FILE->UI) less
thread hops, whee!

BUG=622464
Test=None, internal only change.

Committed: https://crrev.com/b19fcfe287dbb41ba8ee1e09d660cb27b14d1590
Cr-Commit-Position: refs/heads/master@{#419023}

[lazyboy, 2016-08-31 22:55:01]

lazyboy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Devlin, 2016-08-31 23:20:37]

https://codereview.chromium.org/2301713002/diff/1/extensions/browser/api/exec...
File extensions/browser/api/execute_code_function.cc (right):

https://codereview.chromium.org/2301713002/diff/1/extensions/browser/api/exec...
extensions/browser/api/execute_code_function.cc:236:
content::BrowserThread::FILE, FROM_HERE,
So, previously it looks like for component extensions, we went
UI -> FILE (localize + file path) -> UI
and for "normal" extensions, we went
UI -> FILE (read content) -> UI -> FILE (localize + path) -> UI

Now, for component, we do
UI -> FILE (file path) -> UI -(maybe)> FILE (l10n) -> UI
and for normal
UI -> FILE (content + path) -> UI -(maybe)> FILE (l10n) -> UI

So we've made the normal case better, and the component case potentially worse
(though only if inserting css, etc).  I wholeheartedly agree that this is
probably better overall.

But I wonder if we can do better-er. :)  We need to localize if:
a. this function can insert css
b. there's an extension
c. the file content includes the messaging prefix

a and b can be determined before hopping to the file thread, and c can be
determined on the file thread.  As such, I wonder if we could make all of these
cases just
UI -> FILE (read content, get file path, optionally localize) -> UI

This would be a bit tricky because we then need to do work on the file thread
before hopping back, but it looks like FileReader is used relatively few places.

What would you think of changing file reader to be something like:

FileReader(Resource resource,
           Callback optional_file_thread_callback,
           Callback origin_thread_callback);

FileReader::Start() {
  JumpToFileThread();
}

FileReader::RunOnFile() {
  data = ReadData();
  if (!optional_file_thread_callback.IsNull())
    optional_file_thread_callback.Run(data);
  JumpToUIThread(data);
}

?

[lazyboy, 2016-09-01 03:41:49]

Description was changed from

==========
Remove an unnecessary UI->FILE->UI trip in chrome.tabs.executeScript.

For JavaScript file injection, we do not need to call localization on
FILE thread. We only need one information from FILE thread: the GURL
representing the file's path (file_url_ member). However, for the
most common case, we can already get that information from FileReader.
This CL changes FileReader to return that.

Since injecting JS file using chrome.tabs.executeScript() is so common,
this change should benefit us nicely.

BUG=622464
Test=None, internal only change.
==========

to

==========
Remove some UI->FILE->UI thread hops in ExecuteCodeFunction.

There are 3 things that ExecuteCodeFunction does requires FILE thread:
a) Reading the file content (for non-component files)
b) Retrieving the file GURL of the script file from base::FilePath
c) content l10n (if the script is CSS)

This CL does all of these in on UI->FILE hop and doesn't require
additional FILE thread hop.
Previously, the most common case, i.e. chrome.tabs.executeScript()
with {file:...}, would require UI->FILE->UI->FILE->UI. With this CL
this become UI->FILE->UI.

This CL accomplishes this with adding an optional callback to FileReader,
to specify additional tasks to be run on FILE thread.

So the thread hop changes are:
Component extension's executeScript() with file:... url would stay same.
*All* other means of executing script file:... url, e.g.
chrome.tabs.executeScript(,{file:..}) would require two (UI->FILE->UI) less
thread hops, whee!

BUG=622464
Test=None, internal only change.
==========

[lazyboy, 2016-09-01 03:42:11]

Description was changed from

==========
Remove some UI->FILE->UI thread hops in ExecuteCodeFunction.

There are 3 things that ExecuteCodeFunction does requires FILE thread:
a) Reading the file content (for non-component files)
b) Retrieving the file GURL of the script file from base::FilePath
c) content l10n (if the script is CSS)

This CL does all of these in on UI->FILE hop and doesn't require
additional FILE thread hop.
Previously, the most common case, i.e. chrome.tabs.executeScript()
with {file:...}, would require UI->FILE->UI->FILE->UI. With this CL
this become UI->FILE->UI.

This CL accomplishes this with adding an optional callback to FileReader,
to specify additional tasks to be run on FILE thread.

So the thread hop changes are:
Component extension's executeScript() with file:... url would stay same.
*All* other means of executing script file:... url, e.g.
chrome.tabs.executeScript(,{file:..}) would require two (UI->FILE->UI) less
thread hops, whee!

BUG=622464
Test=None, internal only change.
==========

to

==========
Remove some UI->FILE->UI thread hops in ExecuteCodeFunction.

There are 3 things that ExecuteCodeFunction do requires FILE thread:
a) Reading the file content (for non-component files)
b) Retrieving the file GURL of the script file from base::FilePath
c) content l10n (if the script is CSS)

This CL does all of these in on UI->FILE hop and doesn't require
additional FILE thread hop.
Previously, the most common case, i.e. chrome.tabs.executeScript()
with {file:...}, would require UI->FILE->UI->FILE->UI. With this CL
this become UI->FILE->UI.

This CL accomplishes this with adding an optional callback to FileReader,
to specify additional tasks to be run on FILE thread.

So the thread hop changes are:
Component extension's executeScript() with file:... url would stay same.
*All* other means of executing script file:... url, e.g.
chrome.tabs.executeScript(,{file:..}) would require two (UI->FILE->UI) less
thread hops, whee!

BUG=622464
Test=None, internal only change.
==========

[lazyboy, 2016-09-01 03:42:58]

Description was changed from

==========
Remove some UI->FILE->UI thread hops in ExecuteCodeFunction.

There are 3 things that ExecuteCodeFunction do requires FILE thread:
a) Reading the file content (for non-component files)
b) Retrieving the file GURL of the script file from base::FilePath
c) content l10n (if the script is CSS)

This CL does all of these in on UI->FILE hop and doesn't require
additional FILE thread hop.
Previously, the most common case, i.e. chrome.tabs.executeScript()
with {file:...}, would require UI->FILE->UI->FILE->UI. With this CL
this become UI->FILE->UI.

This CL accomplishes this with adding an optional callback to FileReader,
to specify additional tasks to be run on FILE thread.

So the thread hop changes are:
Component extension's executeScript() with file:... url would stay same.
*All* other means of executing script file:... url, e.g.
chrome.tabs.executeScript(,{file:..}) would require two (UI->FILE->UI) less
thread hops, whee!

BUG=622464
Test=None, internal only change.
==========

to

==========
Remove some UI->FILE->UI thread hops in ExecuteCodeFunction.

There are 3 types of ExecuteCodeFunction work, that require FILE thread:
a) Reading the file content (for non-component files)
b) Retrieving the file GURL of the script file from base::FilePath
c) content l10n (if the script is CSS)

This CL does all of these in on UI->FILE hop and doesn't require
additional FILE thread hop.
Previously, the most common case, i.e. chrome.tabs.executeScript()
with {file:...}, would require UI->FILE->UI->FILE->UI. With this CL
this become UI->FILE->UI.

This CL accomplishes this with adding an optional callback to FileReader,
to specify additional tasks to be run on FILE thread.

So the thread hop changes are:
Component extension's executeScript() with file:... url would stay same.
*All* other means of executing script file:... url, e.g.
chrome.tabs.executeScript(,{file:..}) would require two (UI->FILE->UI) less
thread hops, whee!

BUG=622464
Test=None, internal only change.
==========

[lazyboy, 2016-09-01 03:43:39]

Description was changed from

==========
Remove some UI->FILE->UI thread hops in ExecuteCodeFunction.

There are 3 types of ExecuteCodeFunction work, that require FILE thread:
a) Reading the file content (for non-component files)
b) Retrieving the file GURL of the script file from base::FilePath
c) content l10n (if the script is CSS)

This CL does all of these in on UI->FILE hop and doesn't require
additional FILE thread hop.
Previously, the most common case, i.e. chrome.tabs.executeScript()
with {file:...}, would require UI->FILE->UI->FILE->UI. With this CL
this become UI->FILE->UI.

This CL accomplishes this with adding an optional callback to FileReader,
to specify additional tasks to be run on FILE thread.

So the thread hop changes are:
Component extension's executeScript() with file:... url would stay same.
*All* other means of executing script file:... url, e.g.
chrome.tabs.executeScript(,{file:..}) would require two (UI->FILE->UI) less
thread hops, whee!

BUG=622464
Test=None, internal only change.
==========

to

==========
Remove some UI->FILE->UI thread hops in ExecuteCodeFunction.

There are 3 types of tasks in ExecuteCodeFunction that require FILE thread:
a) Reading the file content (for non-component files)
b) Retrieving the file GURL of the script file from base::FilePath
c) content l10n (if the script is CSS)

This CL does all of these in on UI->FILE hop and doesn't require
additional FILE thread hop.
Previously, the most common case, i.e. chrome.tabs.executeScript()
with {file:...}, would require UI->FILE->UI->FILE->UI. With this CL
this become UI->FILE->UI.

This CL accomplishes this with adding an optional callback to FileReader,
to specify additional tasks to be run on FILE thread.

So the thread hop changes are:
Component extension's executeScript() with file:... url would stay same.
*All* other means of executing script file:... url, e.g.
chrome.tabs.executeScript(,{file:..}) would require two (UI->FILE->UI) less
thread hops, whee!

BUG=622464
Test=None, internal only change.
==========

[lazyboy, 2016-09-01 03:44:57]

Description was changed from

==========
Remove some UI->FILE->UI thread hops in ExecuteCodeFunction.

There are 3 types of tasks in ExecuteCodeFunction that require FILE thread:
a) Reading the file content (for non-component files)
b) Retrieving the file GURL of the script file from base::FilePath
c) content l10n (if the script is CSS)

This CL does all of these in on UI->FILE hop and doesn't require
additional FILE thread hop.
Previously, the most common case, i.e. chrome.tabs.executeScript()
with {file:...}, would require UI->FILE->UI->FILE->UI. With this CL
this become UI->FILE->UI.

This CL accomplishes this with adding an optional callback to FileReader,
to specify additional tasks to be run on FILE thread.

So the thread hop changes are:
Component extension's executeScript() with file:... url would stay same.
*All* other means of executing script file:... url, e.g.
chrome.tabs.executeScript(,{file:..}) would require two (UI->FILE->UI) less
thread hops, whee!

BUG=622464
Test=None, internal only change.
==========

to

==========
Remove some UI->FILE->UI thread hops in ExecuteCodeFunction.

There are 3 types of tasks in ExecuteCodeFunction that require FILE thread:
a) Reading the file content (for non-component extension files)
b) Retrieving the file GURL of the script file from base::FilePath
c) content l10n (if the script is CSS)

This CL does all of these in on UI->FILE hop and doesn't require
additional FILE thread hop.
Previously, the most common case, i.e. chrome.tabs.executeScript()
with {file:...}, would require UI->FILE->UI->FILE->UI. With this CL
this become UI->FILE->UI.

This CL accomplishes this with adding an optional callback to FileReader,
to specify additional tasks to be run on FILE thread.

So the thread hop changes are:
Component extension's executeScript() with file:... url would stay same.
*All* other means of executing script file:... url, e.g.
chrome.tabs.executeScript(,{file:..}) would require two (UI->FILE->UI) less
thread hops, whee!

BUG=622464
Test=None, internal only change.
==========

[lazyboy, 2016-09-01 03:52:39]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 03:52:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lazyboy, 2016-09-01 03:52:47]

https://codereview.chromium.org/2301713002/diff/1/extensions/browser/api/exec...
File extensions/browser/api/execute_code_function.cc (right):

https://codereview.chromium.org/2301713002/diff/1/extensions/browser/api/exec...
extensions/browser/api/execute_code_function.cc:236:
content::BrowserThread::FILE, FROM_HERE,
On 2016/08/31 23:20:37, Devlin wrote:
> So, previously it looks like for component extensions, we went
> UI -> FILE (localize + file path) -> UI
> and for "normal" extensions, we went
> UI -> FILE (read content) -> UI -> FILE (localize + path) -> UI
> 
> Now, for component, we do
> UI -> FILE (file path) -> UI -(maybe)> FILE (l10n) -> UI
> and for normal
> UI -> FILE (content + path) -> UI -(maybe)> FILE (l10n) -> UI
> 
> So we've made the normal case better, and the component case potentially worse
> (though only if inserting css, etc).  I wholeheartedly agree that this is
> probably better overall.
> 
> But I wonder if we can do better-er. :)  We need to localize if:
> a. this function can insert css
> b. there's an extension
> c. the file content includes the messaging prefix
> 
> a and b can be determined before hopping to the file thread, and c can be
> determined on the file thread.  As such, I wonder if we could make all of
these
> cases just
> UI -> FILE (read content, get file path, optionally localize) -> UI
> 
> This would be a bit tricky because we then need to do work on the file thread
> before hopping back, but it looks like FileReader is used relatively few
places.
> 
> What would you think of changing file reader to be something like:
> 
> FileReader(Resource resource,
>            Callback optional_file_thread_callback,
>            Callback origin_thread_callback);
> 
> FileReader::Start() {
>   JumpToFileThread();
> }
> 
> FileReader::RunOnFile() {
>   data = ReadData();
>   if (!optional_file_thread_callback.IsNull())
>     optional_file_thread_callback.Run(data);
>   JumpToUIThread(data);
> }
> 
> ?

Done. PTAL, thanks!

[commit-bot: I haz the power, 2016-09-01 04:01:43]

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

[commit-bot: I haz the power, 2016-09-01 04:01:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2016-09-01 04:21:25]

Looks like there's a compile error, but I think it's just the unit tests.

Overall, this looks good.  Great that we knocked down so many thread jumps!

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
File extensions/browser/api/execute_code_function.cc (right):

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:33: bool
HasLocalizationData(const std::string* data) {
nit: const& instead of const* if we keep this.  That said, it's only called
once, so I think it'd be just as readable to say
bool has_localization_data =
    data->find(...) != std::string::npos;

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:54: bool success,
If we make the change to not call this when reading the file doesn't succeed, we
don't need this parameter, right?

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:60: if (!success ||
!requires_localization || HasLocalizationData(data))
is this supposed to be "!HasLocalizationData"?

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:67: // We need to do message
replacement on the data, so it has to be mutable.
nit: I don't this comment adds anything anymore.

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:84: // Call back
DidLoadAndLocalizeFile on the UI thread. The success parameter
nit: The "Call back DidLoadAndLocalizeFile on the UI thread." doesn't tell us
anything that we didn't know from looking at the code - I'd remove it.

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:85: // is always true, because
we've loaded the content from ResourceBundle.
nitty nit: this could be self-documenting in the call to
GetFileUrlAndMaybeLocalize if we did something like
// |success| is always true because we've loaded the content from
ResourceBundle.
const bool success = true;
GetFileUrlAndMaybeLocalize(id, path, locale, needs, success, data);
PostTask(...this, success, ...);

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:204: bool requires_localization
= ShouldInsertCSS() && !extension_id.empty();
s/requires_localization/might_require_localization (since we don't know for sure
yet).  I'd recommend this for the parameter name in the function, too.

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
File extensions/browser/api/execute_code_function.h (right):

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
extensions/browser/api/execute_code_function.h:55: // Retrieves GURL from
filepath and optionally localizes |data|.
nitty nits: Retrieves the file url for the given |extension_path|...

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
extensions/browser/api/execute_code_function.h:59: void
GetFileURLAndMaybeLocalize(const std::string& extension_id,
optional nit: Since we're renaming all these, I find it much more clear when the
method itself has the loop if it's unclear, e.g.
GetFileUrlAndMaybeLocalizeOnFileThread.

It's a mouthful, but if it saves me needing to read the comment in the header,
it might save time overall.

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/file...
File extensions/browser/file_reader.h (right):

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/file...
extensions/browser/file_reader.h:25: // If the file reading doesn't succeed,
this will be ignored.
This makes sense to me, but the code seems to disagree.

[lazyboy, 2016-09-01 05:52:58]

Fix extensions_unittests target, comments addressed in patch set #5

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
File extensions/browser/api/execute_code_function.cc (right):

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:33: bool
HasLocalizationData(const std::string* data) {
On 2016/09/01 04:21:25, Devlin wrote:
> nit: const& instead of const* if we keep this.  That said, it's only called
> once, so I think it'd be just as readable to say
> bool has_localization_data =
>     data->find(...) != std::string::npos;

Removed, originally I had multiple places that needed this.

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:54: bool success,
On 2016/09/01 04:21:25, Devlin wrote:
> If we make the change to not call this when reading the file doesn't succeed,
we
> don't need this parameter, right?

Yes,
I originally intended to do that (hence the stray comment in file_reader.h),
however, I thought calling this always makes things deterministic, e.g. we can
always set file_url_ below.
However, what's the point of setting file_url_ if file load is going to fail...
Fixed.

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:60: if (!success ||
!requires_localization || HasLocalizationData(data))
On 2016/09/01 04:21:25, Devlin wrote:
> is this supposed to be "!HasLocalizationData"?

Yes, didn't check other than compiling on last patch set :(
Done.

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:67: // We need to do message
replacement on the data, so it has to be mutable.
On 2016/09/01 04:21:25, Devlin wrote:
> nit: I don't this comment adds anything anymore.

Done.

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:84: // Call back
DidLoadAndLocalizeFile on the UI thread. The success parameter
On 2016/09/01 04:21:25, Devlin wrote:
> nit: The "Call back DidLoadAndLocalizeFile on the UI thread." doesn't tell us
> anything that we didn't know from looking at the code - I'd remove it.

Done.

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:85: // is always true, because
we've loaded the content from ResourceBundle.
On 2016/09/01 04:21:25, Devlin wrote:
> nitty nit: this could be self-documenting in the call to
> GetFileUrlAndMaybeLocalize if we did something like
> // |success| is always true because we've loaded the content from
> ResourceBundle.
> const bool success = true;
> GetFileUrlAndMaybeLocalize(id, path, locale, needs, success, data);
> PostTask(...this, success, ...);

Done.

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:204: bool requires_localization
= ShouldInsertCSS() && !extension_id.empty();
On 2016/09/01 04:21:25, Devlin wrote:
> s/requires_localization/might_require_localization (since we don't know for
sure
> yet).  I'd recommend this for the parameter name in the function, too.

Done.

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
File extensions/browser/api/execute_code_function.h (right):

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
extensions/browser/api/execute_code_function.h:55: // Retrieves GURL from
filepath and optionally localizes |data|.
On 2016/09/01 04:21:25, Devlin wrote:
> nitty nits: Retrieves the file url for the given |extension_path|...

Done.

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/api/...
extensions/browser/api/execute_code_function.h:59: void
GetFileURLAndMaybeLocalize(const std::string& extension_id,
On 2016/09/01 04:21:25, Devlin wrote:
> optional nit: Since we're renaming all these, I find it much more clear when
the
> method itself has the loop if it's unclear, e.g.
> GetFileUrlAndMaybeLocalizeOnFileThread.
> 
> It's a mouthful, but if it saves me needing to read the comment in the header,
> it might save time overall.

Done.

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/file...
File extensions/browser/file_reader.h (right):

https://codereview.chromium.org/2301713002/diff/40001/extensions/browser/file...
extensions/browser/file_reader.h:25: // If the file reading doesn't succeed,
this will be ignored.
On 2016/09/01 04:21:25, Devlin wrote:
> This makes sense to me, but the code seems to disagree.

See comment in execute_code_function.cc

[lazyboy, 2016-09-01 05:53:21]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 05:53:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-09-01 06:15:21]

lgtm; a few last comments, but they should be pretty straightforward. :)

https://codereview.chromium.org/2301713002/diff/80001/extensions/browser/api/...
File extensions/browser/api/execute_code_function.cc (right):

https://codereview.chromium.org/2301713002/diff/80001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:58: if
(data->find(extensions::MessageBundle::kMessageBegin) ==
nit: either a comment explaining this or a self-documenting variable (bool
needs_message_substitution) would be in order.

https://codereview.chromium.org/2301713002/diff/80001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:197: std::string extension_id =
extension()->id();
const& should be sufficient here.

https://codereview.chromium.org/2301713002/diff/80001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:202: bool
might_require_localization = ShouldInsertCSS() && !extension_id.empty();
Hmm... can extension_id actually be empty?  Before this was a defensive check if
extension() was null, but I don't think that could ever actually happen.

https://codereview.chromium.org/2301713002/diff/80001/extensions/browser/api/...
File extensions/browser/api/execute_code_function.h (right):

https://codereview.chromium.org/2301713002/diff/80001/extensions/browser/api/...
extensions/browser/api/execute_code_function.h:66: void
GetFileURLAndLocalizeComponentResourceOnFileThread(
nit: a brief comment documenting this would be good.

https://codereview.chromium.org/2301713002/diff/80001/extensions/browser/file...
File extensions/browser/file_reader.cc (right):

https://codereview.chromium.org/2301713002/diff/80001/extensions/browser/file...
extensions/browser/file_reader.cc:40: base::Bind(done_callback_, success,
base::Passed(std::move(data))));
Totally separate, but let's make these (here and line 36)
base::ResetAndReturn(&callback_).  Otherwise, we can get into a function
ownership leak, since ExecuteCodeFunction owns this, and this owns a callback
that has a ref to ExecuteCodeFunction.

[commit-bot: I haz the power, 2016-09-01 06:46:40]

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

[commit-bot: I haz the power, 2016-09-01 06:46:41]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2016-09-01 18:08:05]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 18:08:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lazyboy, 2016-09-01 18:10:17]

lazyboy@chromium.org changed reviewers:
+ dtseng@chromium.org

[lazyboy, 2016-09-01 18:10:18]

+David for accessibility_manager.cc OWNERS.

Comments addressed in patch set #6

https://codereview.chromium.org/2301713002/diff/80001/extensions/browser/api/...
File extensions/browser/api/execute_code_function.cc (right):

https://codereview.chromium.org/2301713002/diff/80001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:58: if
(data->find(extensions::MessageBundle::kMessageBegin) ==
On 2016/09/01 06:15:21, Devlin wrote:
> nit: either a comment explaining this or a self-documenting variable (bool
> needs_message_substitution) would be in order.

Done.

https://codereview.chromium.org/2301713002/diff/80001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:197: std::string extension_id =
extension()->id();
On 2016/09/01 06:15:21, Devlin wrote:
> const& should be sufficient here.

Done.

https://codereview.chromium.org/2301713002/diff/80001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:202: bool
might_require_localization = ShouldInsertCSS() && !extension_id.empty();
On 2016/09/01 06:15:21, Devlin wrote:
> Hmm... can extension_id actually be empty?  Before this was a defensive check
if
> extension() was null,

That's probably because we checked extension() in other thread?

 but I don't think that could ever actually happen.

If extension() is not null, then extension_id is guaranteed to be defined? I've
put a todo for this, to avoid more behavior changes in this CL.

https://codereview.chromium.org/2301713002/diff/80001/extensions/browser/api/...
File extensions/browser/api/execute_code_function.h (right):

https://codereview.chromium.org/2301713002/diff/80001/extensions/browser/api/...
extensions/browser/api/execute_code_function.h:66: void
GetFileURLAndLocalizeComponentResourceOnFileThread(
On 2016/09/01 06:15:21, Devlin wrote:
> nit: a brief comment documenting this would be good.

Done.

https://codereview.chromium.org/2301713002/diff/80001/extensions/browser/file...
File extensions/browser/file_reader.cc (right):

https://codereview.chromium.org/2301713002/diff/80001/extensions/browser/file...
extensions/browser/file_reader.cc:40: base::Bind(done_callback_, success,
base::Passed(std::move(data))));
On 2016/09/01 06:15:21, Devlin wrote:
> Totally separate, but let's make these (here and line 36)
> base::ResetAndReturn(&callback_).  Otherwise, we can get into a function
> ownership leak, since ExecuteCodeFunction owns this, and this owns a callback
> that has a ref to ExecuteCodeFunction.

As discussed on chat, there shouldn't be a leak as nobody directly owns
FileReader at this point.
However, added it anyway to defend against future leak possibilities.

[Devlin, 2016-09-01 18:15:42]

https://codereview.chromium.org/2301713002/diff/80001/extensions/browser/api/...
File extensions/browser/api/execute_code_function.cc (right):

https://codereview.chromium.org/2301713002/diff/80001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:202: bool
might_require_localization = ShouldInsertCSS() && !extension_id.empty();
On 2016/09/01 18:10:18, lazyboy wrote:
> On 2016/09/01 06:15:21, Devlin wrote:
> > Hmm... can extension_id actually be empty?  Before this was a defensive
check
> if
> > extension() was null,
> 
> That's probably because we checked extension() in other thread?
> 
>  but I don't think that could ever actually happen.
> 
> If extension() is not null, then extension_id is guaranteed to be defined?
I've
> put a todo for this, to avoid more behavior changes in this CL.

extension_id should definitely always be defined for all extensions.  Otherwise
much badness ensues (e.g., failure to properly place the extension in a set,
look it up in the registry, etc).  It should only ever happen in very small unit
tests, and even then, rarely.  A TODO for now is fine, but I'd like to migrate
it to a DCHECK.

[commit-bot: I haz the power, 2016-09-01 19:03:07]

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

[commit-bot: I haz the power, 2016-09-01 19:03:08]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2016-09-07 20:18:57]

@dtseng, ping.

[lazyboy, 2016-09-14 23:24:29]

lazyboy@chromium.org changed reviewers:
+ dmazzoni@chromium.org
- dtseng@chromium.org

[lazyboy, 2016-09-14 23:24:30]

+dmazzoni@chromium.org for review instead:
chrome/browser/chromeos/accessibility/*

[dmazzoni, 2016-09-15 16:33:19]

lgtm

[lazyboy, 2016-09-15 21:14:23]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 21:15:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 22:10:47]

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

[commit-bot: I haz the power, 2016-09-15 22:10:50]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2016-09-15 22:41:31]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2016-09-15 22:41:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2301713002/#ps140001
(title: "sync, move changes from accessibility_manager.cc ->
accessibility_extension_loader.cc")

[commit-bot: I haz the power, 2016-09-15 22:41:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 22:48:16]

Description was changed from

==========
Remove some UI->FILE->UI thread hops in ExecuteCodeFunction.

There are 3 types of tasks in ExecuteCodeFunction that require FILE thread:
a) Reading the file content (for non-component extension files)
b) Retrieving the file GURL of the script file from base::FilePath
c) content l10n (if the script is CSS)

This CL does all of these in on UI->FILE hop and doesn't require
additional FILE thread hop.
Previously, the most common case, i.e. chrome.tabs.executeScript()
with {file:...}, would require UI->FILE->UI->FILE->UI. With this CL
this become UI->FILE->UI.

This CL accomplishes this with adding an optional callback to FileReader,
to specify additional tasks to be run on FILE thread.

So the thread hop changes are:
Component extension's executeScript() with file:... url would stay same.
*All* other means of executing script file:... url, e.g.
chrome.tabs.executeScript(,{file:..}) would require two (UI->FILE->UI) less
thread hops, whee!

BUG=622464
Test=None, internal only change.
==========

to

==========
Remove some UI->FILE->UI thread hops in ExecuteCodeFunction.

There are 3 types of tasks in ExecuteCodeFunction that require FILE thread:
a) Reading the file content (for non-component extension files)
b) Retrieving the file GURL of the script file from base::FilePath
c) content l10n (if the script is CSS)

This CL does all of these in on UI->FILE hop and doesn't require
additional FILE thread hop.
Previously, the most common case, i.e. chrome.tabs.executeScript()
with {file:...}, would require UI->FILE->UI->FILE->UI. With this CL
this become UI->FILE->UI.

This CL accomplishes this with adding an optional callback to FileReader,
to specify additional tasks to be run on FILE thread.

So the thread hop changes are:
Component extension's executeScript() with file:... url would stay same.
*All* other means of executing script file:... url, e.g.
chrome.tabs.executeScript(,{file:..}) would require two (UI->FILE->UI) less
thread hops, whee!

BUG=622464
Test=None, internal only change.
==========

[commit-bot: I haz the power, 2016-09-15 22:48:17]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-09-15 22:50:23]

Description was changed from

==========
Remove some UI->FILE->UI thread hops in ExecuteCodeFunction.

There are 3 types of tasks in ExecuteCodeFunction that require FILE thread:
a) Reading the file content (for non-component extension files)
b) Retrieving the file GURL of the script file from base::FilePath
c) content l10n (if the script is CSS)

This CL does all of these in on UI->FILE hop and doesn't require
additional FILE thread hop.
Previously, the most common case, i.e. chrome.tabs.executeScript()
with {file:...}, would require UI->FILE->UI->FILE->UI. With this CL
this become UI->FILE->UI.

This CL accomplishes this with adding an optional callback to FileReader,
to specify additional tasks to be run on FILE thread.

So the thread hop changes are:
Component extension's executeScript() with file:... url would stay same.
*All* other means of executing script file:... url, e.g.
chrome.tabs.executeScript(,{file:..}) would require two (UI->FILE->UI) less
thread hops, whee!

BUG=622464
Test=None, internal only change.
==========

to

==========
Remove some UI->FILE->UI thread hops in ExecuteCodeFunction.

There are 3 types of tasks in ExecuteCodeFunction that require FILE thread:
a) Reading the file content (for non-component extension files)
b) Retrieving the file GURL of the script file from base::FilePath
c) content l10n (if the script is CSS)

This CL does all of these in on UI->FILE hop and doesn't require
additional FILE thread hop.
Previously, the most common case, i.e. chrome.tabs.executeScript()
with {file:...}, would require UI->FILE->UI->FILE->UI. With this CL
this become UI->FILE->UI.

This CL accomplishes this with adding an optional callback to FileReader,
to specify additional tasks to be run on FILE thread.

So the thread hop changes are:
Component extension's executeScript() with file:... url would stay same.
*All* other means of executing script file:... url, e.g.
chrome.tabs.executeScript(,{file:..}) would require two (UI->FILE->UI) less
thread hops, whee!

BUG=622464
Test=None, internal only change.

Committed: https://crrev.com/b19fcfe287dbb41ba8ee1e09d660cb27b14d1590
Cr-Commit-Position: refs/heads/master@{#419023}
==========

[commit-bot: I haz the power, 2016-09-15 22:50:24]

Patchset 8 (id:??) landed as
https://crrev.com/b19fcfe287dbb41ba8ee1e09d660cb27b14d1590
Cr-Commit-Position: refs/heads/master@{#419023}