Implement <webview>.addContentScript/removeContentScript API [2]

Implement <webview>.addContentScript/removeContentScript API [2]

This patch includes the changes that enables
<webview>.addContentScript/removeContentScript API  work on WebUI.

-- Introduce WebUIURLFetcherGroup to manage a set of
WebUIURLFetcher and response after all fetches are
done.
-- Move WebUIURLFetcher from web_view_internal_api.cc to a
separate file.
-- A refactoring is done in UserScriptLoader and
ExtensionUserScriptLoader. Since webUI has different ways
to load user script as extension does, so I revert some old
refactoring in ExtensionUserScriptLoader and move all the
loading related code from UserScriptLoader to
ExtensionUserScriptLoader.

This is the second patch in a series of patches:
1) Implement <webview>.addContentScript/removeContentScript API [1] (https://codereview.chromium.org/959413003)
2) Implement <webview>.addContentScript/removeContentScript API [2] (https://codereview.chromium.org/1056533002)
3) Implement <webview>.addContentScript/removeContentScript API [3] (https://codereview.chromium.org/1058113002)

BUG=461052
Committed: https://crrev.com/feb6a64f153422f71952ec3a7cd4fc3c1a90e272
Cr-Commit-Position: refs/heads/master@{#326854}

[Xi Han, 2015-04-01 15:28:37]

Patchset #1 (id:1) has been deleted

[Xi Han, 2015-04-01 15:28:48]

Patchset #1 (id:20001) has been deleted

[Xi Han, 2015-04-01 15:43:04]

hanxi@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Xi Han, 2015-04-01 15:43:05]

Hi Fady: this is the follow-up patch that makes the API work on webUI, PTAL.
Thanks.

[Xi Han, 2015-04-02 15:42:13]

Patchset #2 (id:60001) has been deleted

[Xi Han, 2015-04-08 22:09:30]

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

[Xi Han, 2015-04-08 22:09:30]

rdevlin.cronin@chromium.org: Fady is not familiar with these code, so I send
this CL directly to you. Thanks for reviewing it.

[Xi Han, 2015-04-09 20:19:30]

Patchset #4 (id:120001) has been deleted

[Xi Han, 2015-04-09 20:30:46]

Patchset #5 (id:160001) has been deleted

[Xi Han, 2015-04-09 20:30:59]

Patchset #4 (id:140001) has been deleted

[Xi Han, 2015-04-09 21:43:13]

Hi Fady: as discussed offline, I moved the new added files to  web_view
directory. PTAL, thanks.

[Xi Han, 2015-04-10 20:41:57]

Patchset #6 (id:220001) has been deleted

[Xi Han, 2015-04-15 12:41:05]

Patchset #6 (id:240001) has been deleted

[Fady Samuel, 2015-04-16 19:30:56]

I don't fully understand the URLFetcher code so I can't really review that, but
overall lgtm, Devlin or Xiyuan might be able to give a more meaningful review.

[Xi Han, 2015-04-16 19:32:46]

hanxi@chromium.org changed reviewers:
+ xiyuan@chromium.org

[Xi Han, 2015-04-16 19:32:46]

+ xiyuan@chromium.org: Please review webUI related changes.

[Devlin, 2015-04-16 20:19:29]

High-level comments first.

https://codereview.chromium.org/1056533002/diff/280001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher_group.h
(right):

https://codereview.chromium.org/1056533002/diff/280001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher_group.h:23:
class WebUIURLFetcherGroup : public base::RefCounted<WebUIURLFetcherGroup> {
Can you think of a way this could avoid being refcounted?

https://codereview.chromium.org/1056533002/diff/280001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher_group.h:51:
ScopedVector<WebUIURLFetcher> fetchers_;
Since WebUIURLFetcher is pretty much just a tiny wrapper around URLFetcher,
would it be possible to instead make WebUIURLFetcher support multiple requests,
and get rid of this class?

https://codereview.chromium.org/1056533002/diff/280001/extensions/browser/use...
File extensions/browser/user_script_loader.h (right):

https://codereview.chromium.org/1056533002/diff/280001/extensions/browser/use...
extensions/browser/user_script_loader.h:85: struct UserScriptRenderInfo {
(Almost all of) this is a layering violation. The UserScriptLoader base class
shouldn't have any knowledge of webui, and shouldn't need to store the script
info like this.

[xiyuan, 2015-04-16 21:07:58]

https://codereview.chromium.org/1056533002/diff/280001/chrome/test/data/webui...
File chrome/test/data/webui/webview_execute_script_test.js (right):

https://codereview.chromium.org/1056533002/diff/280001/chrome/test/data/webui...
chrome/test/data/webui/webview_execute_script_test.js:5: var
request_to_comm_channel_1 = 'connect';
nit: const var name should be all upper case.
  request_to_comm_channel_1 -> REQUEST_TO_COMM_CHANNEL_1

https://codereview.chromium.org/1056533002/diff/280001/chrome/test/data/webui...
chrome/test/data/webui/webview_execute_script_test.js:66: [{"name": 'myrule',
nit: " -> '

https://codereview.chromium.org/1056533002/diff/280001/chrome/test/data/webui...
chrome/test/data/webui/webview_execute_script_test.js:78: var data =
JSON.parse(e.data);
'message' event is also used by chrome://sign-in/ page. JSON.parse would fail
because e.data could be an object sent from sign-in page's component
(iframe/webview) and you get "Uncaught SyntaxError: Unexpected token o" error.

We need to do filtering here (and other 'message' handlers), e.g. 

if (e.source != webview.contentWindow)
  return;

or since you are expecting a string, you can check typeof e.data == 'string'
etc.

[Xi Han, 2015-04-17 20:55:21]

Patchset #10 (id:340001) has been deleted

[Xi Han, 2015-04-17 21:00:00]

Patchset #9 (id:320001) has been deleted

[Xi Han, 2015-04-17 21:00:19]

Patchset #9 (id:360001) has been deleted

[Xi Han, 2015-04-17 21:27:47]

Patchset #9 (id:380001) has been deleted

[Xi Han, 2015-04-17 21:41:16]

Patchset #9 (id:400001) has been deleted

[Xi Han, 2015-04-17 21:44:21]

Take sometimes to answer all the questions and update my CL, PTAL, thanks.

https://codereview.chromium.org/1056533002/diff/280001/chrome/test/data/webui...
File chrome/test/data/webui/webview_execute_script_test.js (right):

https://codereview.chromium.org/1056533002/diff/280001/chrome/test/data/webui...
chrome/test/data/webui/webview_execute_script_test.js:5: var
request_to_comm_channel_1 = 'connect';
On 2015/04/16 21:07:58, xiyuan wrote:
> nit: const var name should be all upper case.
>   request_to_comm_channel_1 -> REQUEST_TO_COMM_CHANNEL_1

Done.

https://codereview.chromium.org/1056533002/diff/280001/chrome/test/data/webui...
chrome/test/data/webui/webview_execute_script_test.js:66: [{"name": 'myrule',
On 2015/04/16 21:07:58, xiyuan wrote:
> nit: " -> '

Hmmm, for this API, we follow the way that extension's content scripts do
(https://developer.chrome.com/extensions/content_scripts). If you see their
example, they do put "" around these properties. How about we still keep the ""
here?

https://codereview.chromium.org/1056533002/diff/280001/chrome/test/data/webui...
chrome/test/data/webui/webview_execute_script_test.js:78: var data =
JSON.parse(e.data);
On 2015/04/16 21:07:58, xiyuan wrote:
> 'message' event is also used by chrome://sign-in/ page. JSON.parse would fail
> because e.data could be an object sent from sign-in page's component
> (iframe/webview) and you get "Uncaught SyntaxError: Unexpected token o" error.
> 
> We need to do filtering here (and other 'message' handlers), e.g. 
> 
> if (e.source != webview.contentWindow)
>   return;
> 
> or since you are expecting a string, you can check typeof e.data == 'string'
> etc.

Good catch, I do see test failures on ChromeOS. Thanks for letting me know the
reason. Fixed all the places.

https://codereview.chromium.org/1056533002/diff/280001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher_group.h
(right):

https://codereview.chromium.org/1056533002/diff/280001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher_group.h:23:
class WebUIURLFetcherGroup : public base::RefCounted<WebUIURLFetcherGroup> {
On 2015/04/16 20:19:28, Devlin wrote:
> Can you think of a way this could avoid being refcounted?

It needs to be RefCounted since in web_ui_url_fetcher_group.cc (line 26) we can
bind WebUIURLFtcherGroup::OnWebUIURLFetchComplete() and a pointer of
WebUIURLFtcherGroup as the call back to create a new WebUIURLFetcher.

https://codereview.chromium.org/1056533002/diff/280001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher_group.h:51:
ScopedVector<WebUIURLFetcher> fetchers_;
On 2015/04/16 20:19:28, Devlin wrote:
> Since WebUIURLFetcher is pretty much just a tiny wrapper around URLFetcher,
> would it be possible to instead make WebUIURLFetcher support multiple
requests,
> and get rid of this class?

As discussed offline, I still adopt the way to keep this group to organize a
group of fetches.

https://codereview.chromium.org/1056533002/diff/280001/extensions/browser/use...
File extensions/browser/user_script_loader.h (right):

https://codereview.chromium.org/1056533002/diff/280001/extensions/browser/use...
extensions/browser/user_script_loader.h:85: struct UserScriptRenderInfo {
On 2015/04/16 20:19:28, Devlin wrote:
> (Almost all of) this is a layering violation. The UserScriptLoader base class
> shouldn't have any knowledge of webui, and shouldn't need to store the script
> info like this.

You are right. Refactoring is done to move all these webui-related routing info
to WebUIuserScriptLoader.

[Devlin, 2015-04-17 22:05:06]

https://codereview.chromium.org/1056533002/diff/280001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher_group.h
(right):

https://codereview.chromium.org/1056533002/diff/280001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher_group.h:23:
class WebUIURLFetcherGroup : public base::RefCounted<WebUIURLFetcherGroup> {
On 2015/04/17 21:44:20, Xi Han wrote:
> On 2015/04/16 20:19:28, Devlin wrote:
> > Can you think of a way this could avoid being refcounted?
> 
> It needs to be RefCounted since in web_ui_url_fetcher_group.cc (line 26) we
can
> bind WebUIURLFtcherGroup::OnWebUIURLFetchComplete() and a pointer of
> WebUIURLFtcherGroup as the call back to create a new WebUIURLFetcher. 

The callback will be called by the WebUIURLFetcher, which is owned by this
class.  If the WebUIURLFetcherGroup is destroyed, the WebUIURLFetcher is
destroyed, and there is nothing to call the callback.  Right?

https://codereview.chromium.org/1056533002/diff/420001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/420001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:117:
fetcher_group->Start(file_set, info.render_process_id, info.render_view_id);
In what circumstance will we ever have more than one fetch running at a time? 
StartLoad() is called gated on whether or not we are currently loading....

[Xi Han, 2015-04-17 22:28:24]

https://codereview.chromium.org/1056533002/diff/280001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher_group.h
(right):

https://codereview.chromium.org/1056533002/diff/280001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher_group.h:23:
class WebUIURLFetcherGroup : public base::RefCounted<WebUIURLFetcherGroup> {
On 2015/04/17 22:05:05, Devlin wrote:
> On 2015/04/17 21:44:20, Xi Han wrote:
> > On 2015/04/16 20:19:28, Devlin wrote:
> > > Can you think of a way this could avoid being refcounted?
> > 
> > It needs to be RefCounted since in web_ui_url_fetcher_group.cc (line 26) we
> can
> > bind WebUIURLFtcherGroup::OnWebUIURLFetchComplete() and a pointer of
> > WebUIURLFtcherGroup as the call back to create a new WebUIURLFetcher. 
> 
> The callback will be called by the WebUIURLFetcher, which is owned by this
> class.  If the WebUIURLFetcherGroup is destroyed, the WebUIURLFetcher is
> destroyed, and there is nothing to call the callback.  Right?

I am confused. I assume the WebUIURLFetcherGroup will be destroyed only when the
all of the callbacks are called. Right?

https://codereview.chromium.org/1056533002/diff/420001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/420001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:117:
fetcher_group->Start(file_set, info.render_process_id, info.render_view_id);
On 2015/04/17 22:05:05, Devlin wrote:
> In what circumstance will we ever have more than one fetch running at a time? 
> StartLoad() is called gated on whether or not we are currently loading....

So that means we can assume there is always only one group running? If that is
the case, I can move all the code from the  WebUIURLFetcherGroup to
WebUIUserScriptLoader.

[Devlin, 2015-04-17 22:40:15]

https://codereview.chromium.org/1056533002/diff/280001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher_group.h
(right):

https://codereview.chromium.org/1056533002/diff/280001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher_group.h:23:
class WebUIURLFetcherGroup : public base::RefCounted<WebUIURLFetcherGroup> {
On 2015/04/17 22:28:24, Xi Han wrote:
> On 2015/04/17 22:05:05, Devlin wrote:
> > On 2015/04/17 21:44:20, Xi Han wrote:
> > > On 2015/04/16 20:19:28, Devlin wrote:
> > > > Can you think of a way this could avoid being refcounted?
> > > 
> > > It needs to be RefCounted since in web_ui_url_fetcher_group.cc (line 26)
we
> > can
> > > bind WebUIURLFtcherGroup::OnWebUIURLFetchComplete() and a pointer of
> > > WebUIURLFtcherGroup as the call back to create a new WebUIURLFetcher. 
> > 
> > The callback will be called by the WebUIURLFetcher, which is owned by this
> > class.  If the WebUIURLFetcherGroup is destroyed, the WebUIURLFetcher is
> > destroyed, and there is nothing to call the callback.  Right?
> 
> I am confused. I assume the WebUIURLFetcherGroup will be destroyed only when
the
> all of the callbacks are called. Right?

If you recount it, yes, it will only be destroyed once all references have been
destroyed, including those references in the callbacks.  So if the callbacks are
the last thing there, then yes, it will be destroyed after they all run.  If you
do *not* refcount it, it will be destroyed in the "normal" fashion (being
deleted, going out of scope, etc).  Without refcounting, binding an object does
not extend its lifetime.

https://codereview.chromium.org/1056533002/diff/420001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/420001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:117:
fetcher_group->Start(file_set, info.render_process_id, info.render_view_id);
On 2015/04/17 22:28:24, Xi Han wrote:
> On 2015/04/17 22:05:05, Devlin wrote:
> > In what circumstance will we ever have more than one fetch running at a
time? 
> > StartLoad() is called gated on whether or not we are currently loading....
> 
> So that means we can assume there is always only one group running? If that is
> the case, I can move all the code from the  WebUIURLFetcherGroup to
> WebUIUserScriptLoader.

SGTM.

[xiyuan, 2015-04-17 22:40:55]

https://codereview.chromium.org/1056533002/diff/280001/chrome/test/data/webui...
File chrome/test/data/webui/webview_execute_script_test.js (right):

https://codereview.chromium.org/1056533002/diff/280001/chrome/test/data/webui...
chrome/test/data/webui/webview_execute_script_test.js:66: [{"name": 'myrule',
On 2015/04/17 21:44:20, Xi Han wrote:
> On 2015/04/16 21:07:58, xiyuan wrote:
> > nit: " -> '
> 
> Hmmm, for this API, we follow the way that extension's content scripts do
> (https://developer.chrome.com/extensions/content_scripts). If you see their
> example, they do put "" around these properties. How about we still keep the
""
> here?

It is different. The extension content script section is in manifest.json and
double quote is used in manifest.json. Here it is JS code in the chromium repo.
The style guide prefers single quote.

https://www.chromium.org/developers/web-development-style-guide

Please fix here and all other places.

[Fady Samuel, 2015-04-20 13:01:04]

https://codereview.chromium.org/1056533002/diff/420001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.h (right):

https://codereview.chromium.org/1056533002/diff/420001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.h:5: #ifndef
EXTENSIONS_BROWSER_WEB_UI_URL_FETCHER_H
Update this?

[Xi Han, 2015-04-20 19:41:19]

Patchset #11 (id:460001) has been deleted

[Xi Han, 2015-04-20 19:41:39]

Patchset #10 (id:440001) has been deleted

[Xi Han, 2015-04-20 22:40:10]

Even though there are tests failed, I would like send the update first, so you
can see on high level the changes that I have made. I will figure out the reason
of failures ASAP. Thanks.

https://codereview.chromium.org/1056533002/diff/280001/chrome/test/data/webui...
File chrome/test/data/webui/webview_execute_script_test.js (right):

https://codereview.chromium.org/1056533002/diff/280001/chrome/test/data/webui...
chrome/test/data/webui/webview_execute_script_test.js:66: [{"name": 'myrule',
On 2015/04/17 22:40:55, xiyuan wrote:
> On 2015/04/17 21:44:20, Xi Han wrote:
> > On 2015/04/16 21:07:58, xiyuan wrote:
> > > nit: " -> '
> > 
> > Hmmm, for this API, we follow the way that extension's content scripts do
> > (https://developer.chrome.com/extensions/content_scripts). If you see their
> > example, they do put "" around these properties. How about we still keep the
> ""
> > here?
> 
> It is different. The extension content script section is in manifest.json and
> double quote is used in manifest.json. Here it is JS code in the chromium
repo.
> The style guide prefers single quote.
> 
> https://www.chromium.org/developers/web-development-style-guide
> 
> Please fix here and all other places.

Done.

https://codereview.chromium.org/1056533002/diff/280001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher_group.h
(right):

https://codereview.chromium.org/1056533002/diff/280001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher_group.h:23:
class WebUIURLFetcherGroup : public base::RefCounted<WebUIURLFetcherGroup> {
On 2015/04/17 22:40:15, Devlin wrote:
> On 2015/04/17 22:28:24, Xi Han wrote:
> > On 2015/04/17 22:05:05, Devlin wrote:
> > > On 2015/04/17 21:44:20, Xi Han wrote:
> > > > On 2015/04/16 20:19:28, Devlin wrote:
> > > > > Can you think of a way this could avoid being refcounted?
> > > > 
> > > > It needs to be RefCounted since in web_ui_url_fetcher_group.cc (line 26)
> we
> > > can
> > > > bind WebUIURLFtcherGroup::OnWebUIURLFetchComplete() and a pointer of
> > > > WebUIURLFtcherGroup as the call back to create a new WebUIURLFetcher. 
> > > 
> > > The callback will be called by the WebUIURLFetcher, which is owned by this
> > > class.  If the WebUIURLFetcherGroup is destroyed, the WebUIURLFetcher is
> > > destroyed, and there is nothing to call the callback.  Right?
> > 
> > I am confused. I assume the WebUIURLFetcherGroup will be destroyed only when
> the
> > all of the callbacks are called. Right?
> 
> If you recount it, yes, it will only be destroyed once all references have
been
> destroyed, including those references in the callbacks.  So if the callbacks
are
> the last thing there, then yes, it will be destroyed after they all run.  If
you
> do *not* refcount it, it will be destroyed in the "normal" fashion (being
> deleted, going out of scope, etc).  Without refcounting, binding an object
does
> not extend its lifetime.

Removed.

https://codereview.chromium.org/1056533002/diff/420001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.h (right):

https://codereview.chromium.org/1056533002/diff/420001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.h:5: #ifndef
EXTENSIONS_BROWSER_WEB_UI_URL_FETCHER_H
On 2015/04/20 13:01:04, Fady Samuel wrote:
> Update this?

Done.

https://codereview.chromium.org/1056533002/diff/420001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/420001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:117:
fetcher_group->Start(file_set, info.render_process_id, info.render_view_id);
On 2015/04/17 22:40:15, Devlin wrote:
> On 2015/04/17 22:28:24, Xi Han wrote:
> > On 2015/04/17 22:05:05, Devlin wrote:
> > > In what circumstance will we ever have more than one fetch running at a
> time? 
> > > StartLoad() is called gated on whether or not we are currently loading....
> > 
> > So that means we can assume there is always only one group running? If that
is
> > the case, I can move all the code from the  WebUIURLFetcherGroup to
> > WebUIUserScriptLoader.
> 
> SGTM.

Done.

[xiyuan, 2015-04-20 23:00:19]

webui related LGTM with nits

https://codereview.chromium.org/1056533002/diff/280001/chrome/test/data/webui...
File chrome/test/data/webui/webview_execute_script_test.js (right):

https://codereview.chromium.org/1056533002/diff/280001/chrome/test/data/webui...
chrome/test/data/webui/webview_execute_script_test.js:5: var
request_to_comm_channel_1 = 'connect';
On 2015/04/17 21:44:20, Xi Han wrote:
> On 2015/04/16 21:07:58, xiyuan wrote:
> > nit: const var name should be all upper case.
> >   request_to_comm_channel_1 -> REQUEST_TO_COMM_CHANNEL_1
> 
> Done.

Not really done. :)

https://codereview.chromium.org/1056533002/diff/480001/chrome/test/data/webui...
File chrome/test/data/webui/webview_execute_script_test.js (right):

https://codereview.chromium.org/1056533002/diff/480001/chrome/test/data/webui...
chrome/test/data/webui/webview_execute_script_test.js:182: if (typeof(e.data) !=
'string')
Please be consistent. Let's use

e.source != webview.contentWindow

everywhere.

https://codereview.chromium.org/1056533002/diff/480001/chrome/test/data/webui...
chrome/test/data/webui/webview_execute_script_test.js:348: if (typeof(e.data) !=
'string')
ditto

[Devlin, 2015-04-20 23:48:25]

https://codereview.chromium.org/1056533002/diff/480001/extensions/browser/ext...
File extensions/browser/extension_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/480001/extensions/browser/ext...
extensions/browser/extension_user_script_loader.cc:201: void
ExtensionUserScriptLoader::LoadScripts(
This would be cleaner if we kept the |user_scripts_| in the superclass as much
as possible.  Let's have the superclass pass in the user_scripts_ here, as well
as a callback to OnScriptsLoaded.

https://codereview.chromium.org/1056533002/diff/480001/extensions/browser/ext...
extensions/browser/extension_user_script_loader.cc:227: return;
Why return?  Shouldn't it be continue?

https://codereview.chromium.org/1056533002/diff/480001/extensions/browser/use...
File extensions/browser/user_script_loader.h (right):

https://codereview.chromium.org/1056533002/diff/480001/extensions/browser/use...
extensions/browser/user_script_loader.h:72: void ClearScripts(bool is_clear);
Do we still need this bool?

https://codereview.chromium.org/1056533002/diff/480001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/480001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:72: void
WebUIUserScriptLoader::LoadScripts(const std::set<HostID>& changed_hosts,
See comment in other class about changing this method signature.

https://codereview.chromium.org/1056533002/diff/480001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:76:
std::set<extensions::UserScript::File*> file_set;
unless you need the uniqueness that a set offers, prefer vector.

https://codereview.chromium.org/1056533002/diff/480001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:84: // Since every script in
|added_script_ids| is associated with the same
If every script here is guaranteed to be associated with the same render info,
why do we need a map of render info at all (as opposed to a single instance)?

[Xi Han, 2015-04-21 20:59:47]

Patchset #11 (id:500001) has been deleted

[Xi Han, 2015-04-21 20:59:56]

Patchset #11 (id:520001) has been deleted

[Xi Han, 2015-04-21 21:00:05]

Patchset #11 (id:540001) has been deleted

[Xi Han, 2015-04-21 21:18:58]

There are some refacotring in this patch:
1) make |user_scripts_| as a private member in UserScriptLoader;
2) Refactoring in WebUIURLFatcher to facility start a series of fetches with all
the render info set before calling the Start().
3) Fix the bug called by callback.Reset() in WebUIURLFatcher. Add refCount and
Release() in web_view_internal_execute_function and remove this reset. So in
both WebUIUserScriptLoader and WebViewInternalExecuteCodeFunction, we can use
WebUIURLFatcher without calling callback.Reset().
PTAK, thanks.

https://codereview.chromium.org/1056533002/diff/280001/chrome/test/data/webui...
File chrome/test/data/webui/webview_execute_script_test.js (right):

https://codereview.chromium.org/1056533002/diff/280001/chrome/test/data/webui...
chrome/test/data/webui/webview_execute_script_test.js:5: var
request_to_comm_channel_1 = 'connect';
On 2015/04/20 23:00:18, xiyuan wrote:
> On 2015/04/17 21:44:20, Xi Han wrote:
> > On 2015/04/16 21:07:58, xiyuan wrote:
> > > nit: const var name should be all upper case.
> > >   request_to_comm_channel_1 -> REQUEST_TO_COMM_CHANNEL_1
> > 
> > Done.
> 
> Not really done. :)

Sorry, I lost all the changes of this file when I was trying to revert to the
old patch, and missed this part when adding changes back.

https://codereview.chromium.org/1056533002/diff/480001/chrome/test/data/webui...
File chrome/test/data/webui/webview_execute_script_test.js (right):

https://codereview.chromium.org/1056533002/diff/480001/chrome/test/data/webui...
chrome/test/data/webui/webview_execute_script_test.js:182: if (typeof(e.data) !=
'string')
On 2015/04/20 23:00:18, xiyuan wrote:
> Please be consistent. Let's use
> 
> e.source != webview.contentWindow
> 
> everywhere.

Done.

https://codereview.chromium.org/1056533002/diff/480001/chrome/test/data/webui...
chrome/test/data/webui/webview_execute_script_test.js:348: if (typeof(e.data) !=
'string')
On 2015/04/20 23:00:18, xiyuan wrote:
> ditto

Done.

https://codereview.chromium.org/1056533002/diff/480001/extensions/browser/ext...
File extensions/browser/extension_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/480001/extensions/browser/ext...
extensions/browser/extension_user_script_loader.cc:201: void
ExtensionUserScriptLoader::LoadScripts(
On 2015/04/20 23:48:25, Devlin wrote:
> This would be cleaner if we kept the |user_scripts_| in the superclass as much
> as possible.  Let's have the superclass pass in the user_scripts_ here, as
well
> as a callback to OnScriptsLoaded.

Change |user_scripts_| to a private member of base class.

https://codereview.chromium.org/1056533002/diff/480001/extensions/browser/ext...
extensions/browser/extension_user_script_loader.cc:227: return;
On 2015/04/20 23:48:25, Devlin wrote:
> Why return?  Shouldn't it be continue?

Good catch, thanks.

https://codereview.chromium.org/1056533002/diff/480001/extensions/browser/use...
File extensions/browser/user_script_loader.h (right):

https://codereview.chromium.org/1056533002/diff/480001/extensions/browser/use...
extensions/browser/user_script_loader.h:72: void ClearScripts(bool is_clear);
On 2015/04/20 23:48:25, Devlin wrote:
> Do we still need this bool?

Yes, it is also called by DeclarativeUserScriptMaster which set |clear_scripts_|
to true; while in the loader, we always set |clear_scripts_| to false.

https://codereview.chromium.org/1056533002/diff/480001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/480001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:72: void
WebUIUserScriptLoader::LoadScripts(const std::set<HostID>& changed_hosts,
On 2015/04/20 23:48:25, Devlin wrote:
> See comment in other class about changing this method signature.

Done.

https://codereview.chromium.org/1056533002/diff/480001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:76:
std::set<extensions::UserScript::File*> file_set;
On 2015/04/20 23:48:25, Devlin wrote:
> unless you need the uniqueness that a set offers, prefer vector.

Done.

https://codereview.chromium.org/1056533002/diff/480001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:84: // Since every script in
|added_script_ids| is associated with the same
On 2015/04/20 23:48:25, Devlin wrote:
> If every script here is guaranteed to be associated with the same render info,
> why do we need a map of render info at all (as opposed to a single instance)?

Updated the comment here. After reconsidering it, I think we need this map since
user scripts may be added by calling multiple AddScripts() when loading, so we
cannot guarantee all of them comes from the same render.

[Devlin, 2015-04-21 22:32:21]

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/api...
File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
(right):

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/api...
extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:359:
base::Unretained(this), file)))
I think I liked this better the other way.  It's kind of silly to say AddRef()
and base::Unretained(), and it's easier to have the WebUIUrlFetcher just reset
the callback in all cases than to always make sure you don't have any refs
sitting around.

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/ext...
File extensions/browser/extension_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/ext...
extensions/browser/extension_user_script_loader.cc:227: return;
continue?

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/use...
File extensions/browser/user_script_loader.cc (left):

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/use...
extensions/browser/user_script_loader.cc:396: clear_scripts_ = false;
Why don't we leave these in here, so we can get rid of the bool to ClearScripts?

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/use...
File extensions/browser/user_script_loader.h (right):

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/use...
extensions/browser/user_script_loader.h:140: scoped_ptr<UserScriptList>
user_scripts_;
No reason to move this.  Best to avoid unintentional changes.

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:53: :
UserScriptLoader(browser_context, host_id),
you're missing an initialization here

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:100:
extensions::UserScript::File& file = script.js_scripts()[k];
The fact that you make this temporary implies the for loop would be easier as
for (extensions::UserScript::File& file : scripts.js_scripts()) {
}

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:101: if
(file.GetContent().empty()) {
Let's extract these lines into a lambda, since they're identical.

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:105: base::Unretained(this),
&file)));
You should comment on why the Unretained is safe.

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:158:
user_scripts_cache_.reset(nullptr);
base::Passed should reset the pointer already.

Also, FYI, you can just do ptr.reset() (you don't need to specify nullptr).

[Xi Han, 2015-04-22 15:00:25]

Patchset #12 (id:580001) has been deleted

[Xi Han, 2015-04-22 15:32:11]

PTAL, thanks!

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/api...
File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
(right):

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/api...
extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:359:
base::Unretained(this), file)))
On 2015/04/21 22:32:20, Devlin wrote:
> I think I liked this better the other way.  It's kind of silly to say AddRef()
> and base::Unretained(), and it's easier to have the WebUIUrlFetcher just reset
> the callback in all cases than to always make sure you don't have any refs
> sitting around.

Ok, revert these changes.

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/ext...
File extensions/browser/extension_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/ext...
extensions/browser/extension_user_script_loader.cc:227: return;
On 2015/04/21 22:32:20, Devlin wrote:
> continue?

Sorry, I did updated it here, but might lost, thanks for catching it again.

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/use...
File extensions/browser/user_script_loader.cc (left):

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/use...
extensions/browser/user_script_loader.cc:396: clear_scripts_ = false;
On 2015/04/21 22:32:20, Devlin wrote:
> Why don't we leave these in here, so we can get rid of the bool to
ClearScripts?

Hmm, right, moved it back after LoadScripts were called.

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/use...
File extensions/browser/user_script_loader.h (right):

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/use...
extensions/browser/user_script_loader.h:140: scoped_ptr<UserScriptList>
user_scripts_;
On 2015/04/21 22:32:20, Devlin wrote:
> No reason to move this.  Best to avoid unintentional changes.

Done.

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:53: :
UserScriptLoader(browser_context, host_id),
On 2015/04/21 22:32:20, Devlin wrote:
> you're missing an initialization here

Got it:)

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:100:
extensions::UserScript::File& file = script.js_scripts()[k];
On 2015/04/21 22:32:20, Devlin wrote:
> The fact that you make this temporary implies the for loop would be easier as
> for (extensions::UserScript::File& file : scripts.js_scripts()) {
> }

Copy and paste from old code, updated:)

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:101: if
(file.GetContent().empty()) {
On 2015/04/21 22:32:20, Devlin wrote:
> Let's extract these lines into a lambda, since they're identical.

The whole loops are identical (only the difference is js_files or css_files), we
can remove all these duplications. Since it contains several line, I put them
into a private helper function.

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:105: base::Unretained(this),
&file)));
On 2015/04/21 22:32:20, Devlin wrote:
> You should comment on why the Unretained is safe.

Done.

https://codereview.chromium.org/1056533002/diff/560001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:158:
user_scripts_cache_.reset(nullptr);
On 2015/04/21 22:32:20, Devlin wrote:
> base::Passed should reset the pointer already.
> 
> Also, FYI, you can just do ptr.reset() (you don't need to specify nullptr).

I see, I will update the place wheRE calling user_scripts.reset(NULL).

[Devlin, 2015-04-22 21:15:33]

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/dec...
File extensions/browser/declarative_user_script_master.h (right):

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/dec...
extensions/browser/declarative_user_script_master.h:9: #include
"extensions/browser/user_script_loader.h"
I think a forward declaration would work for this.

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/dec...
extensions/browser/declarative_user_script_master.h:38: int render_process_id,
It's kind of a shame to have the render process/view ids here, because it
shouldn't really be needed.  But the workaround is a lot more effort, and the
layering violation is relatively small.  So how about just
TODO(hanxi): The renderer information doesn't really belong in this base class,
but it's not an easy fix.

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/ext...
File extensions/browser/extension_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/ext...
extensions/browser/extension_user_script_loader.cc:135: for (size_t k = 0; k <
script->js_scripts().size(); ++k) {
range-based for loops (this whole file)

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/ext...
extensions/browser/extension_user_script_loader.cc:198: NULL /* no verifier for
testing */);
prefer nullptr (this whole file)

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.h (right):

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.h:8: #include
"net/url_request/url_fetcher.h"
Forward declaration?

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:39: bool operator<(const
UserScriptRenderInfo& other) {
Why do we need this?

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:79: const std::set<int>&
added_script_ids) {
Let's make this LoadScriptsAndSerialize, and I still think I'd prefer it to take
a callback (so that OnScriptsLoaded() can be private again).

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:97:
CreateWebUIURLFetchers(&script.js_scripts(), render_process_id,
Why not pass by const &?

[Xi Han, 2015-04-22 22:22:49]

Patchset #13 (id:620001) has been deleted

[Xi Han, 2015-04-22 22:33:30]

Patchset #13 (id:640001) has been deleted

[Xi Han, 2015-04-22 22:34:00]

PTAL, thanks!

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/dec...
File extensions/browser/declarative_user_script_master.h (right):

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/dec...
extensions/browser/declarative_user_script_master.h:9: #include
"extensions/browser/user_script_loader.h"
On 2015/04/22 21:15:33, Devlin wrote:
> I think a forward declaration would work for this.

Since we declare scoped_ptr<UserScriptLoader> loader_, we have to include this
file.

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/dec...
extensions/browser/declarative_user_script_master.h:38: int render_process_id,
On 2015/04/22 21:15:33, Devlin wrote:
> It's kind of a shame to have the render process/view ids here, because it
> shouldn't really be needed.  But the workaround is a lot more effort, and the
> layering violation is relatively small.  So how about just
> TODO(hanxi): The renderer information doesn't really belong in this base
class,
> but it's not an easy fix.

Sounds fair to me.

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/ext...
File extensions/browser/extension_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/ext...
extensions/browser/extension_user_script_loader.cc:135: for (size_t k = 0; k <
script->js_scripts().size(); ++k) {
On 2015/04/22 21:15:33, Devlin wrote:
> range-based for loops (this whole file)

Done.

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/ext...
extensions/browser/extension_user_script_loader.cc:198: NULL /* no verifier for
testing */);
On 2015/04/22 21:15:33, Devlin wrote:
> prefer nullptr (this whole file)

Done.

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.h (right):

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.h:8: #include
"net/url_request/url_fetcher.h"
On 2015/04/22 21:15:33, Devlin wrote:
> Forward declaration?

We use scoped_ptr<net::URLFetcher> fetcher_, so we need this include.

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:39: bool operator<(const
UserScriptRenderInfo& other) {
On 2015/04/22 21:15:33, Devlin wrote:
> Why do we need this?

We store the info objects in a map, so it requires to overload the "<" operator.

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:79: const std::set<int>&
added_script_ids) {
On 2015/04/22 21:15:33, Devlin wrote:
> Let's make this LoadScriptsAndSerialize, and I still think I'd prefer it to
take
> a callback (so that OnScriptsLoaded() can be private again).

A compromise is we have to declare the callback type as a public type of
UserScriptLoader, since there are some functions in the anonymous namespace
refers to it.

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:97:
CreateWebUIURLFetchers(&script.js_scripts(), render_process_id,
On 2015/04/22 21:15:33, Devlin wrote:
> Why not pass by const &?

Once the fetch is done, we will modify the file by copying the data to set its
content. We can't pass in a const &.

[Devlin, 2015-04-22 23:20:51]

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/dec...
File extensions/browser/declarative_user_script_master.h (right):

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/dec...
extensions/browser/declarative_user_script_master.h:9: #include
"extensions/browser/user_script_loader.h"
On 2015/04/22 22:33:59, Xi Han wrote:
> On 2015/04/22 21:15:33, Devlin wrote:
> > I think a forward declaration would work for this.
> 
> Since we declare scoped_ptr<UserScriptLoader> loader_, we have to include this
> file.

scoped_ptr requires only a forward declaration, not the whole file.

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.h (right):

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.h:8: #include
"net/url_request/url_fetcher.h"
On 2015/04/22 22:34:00, Xi Han wrote:
> On 2015/04/22 21:15:33, Devlin wrote:
> > Forward declaration?
> 
> We use scoped_ptr<net::URLFetcher> fetcher_, so we need this include.

scoped_ptr needs only a forward declaration.

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:39: bool operator<(const
UserScriptRenderInfo& other) {
On 2015/04/22 22:34:00, Xi Han wrote:
> On 2015/04/22 21:15:33, Devlin wrote:
> > Why do we need this?
> 
> We store the info objects in a map, so it requires to overload the "<"
operator.

Only the key type of a std::map requires the operator<, not the value type.

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:79: const std::set<int>&
added_script_ids) {
On 2015/04/22 22:34:00, Xi Han wrote:
> On 2015/04/22 21:15:33, Devlin wrote:
> > Let's make this LoadScriptsAndSerialize, and I still think I'd prefer it to
> take
> > a callback (so that OnScriptsLoaded() can be private again).
> 
> A compromise is we have to declare the callback type as a public type of
> UserScriptLoader, since there are some functions in the anonymous namespace
> refers to it.

I don't understand.  Why?

LoadUserScriptsAndSerialize(Callback on_complete) {
  callback_ = on_complete;
  FetchStuff();
}

OnFetchComplete() {
  SerializeOnFileThread(callback_);
  callback_.Reset();
}

SerializeOnFileThread(Callback c) {
  Serialize();
  c.Run();
}

[Xi Han, 2015-04-23 14:37:14]

Patchset #14 (id:680001) has been deleted

[Xi Han, 2015-04-23 14:37:30]

Patchset #14 (id:680001) has been deleted

[Xi Han, 2015-04-23 15:01:56]

Hi Devlin, please see my comments to see whether we are on the same page.
Thanks.

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/dec...
File extensions/browser/declarative_user_script_master.h (right):

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/dec...
extensions/browser/declarative_user_script_master.h:9: #include
"extensions/browser/user_script_loader.h"
On 2015/04/22 23:20:50, Devlin wrote:
> On 2015/04/22 22:33:59, Xi Han wrote:
> > On 2015/04/22 21:15:33, Devlin wrote:
> > > I think a forward declaration would work for this.
> > 
> > Since we declare scoped_ptr<UserScriptLoader> loader_, we have to include
this
> > file.
> 
> scoped_ptr requires only a forward declaration, not the whole file.

Done.

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.h (right):

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.h:8: #include
"net/url_request/url_fetcher.h"
On 2015/04/22 23:20:50, Devlin wrote:
> On 2015/04/22 22:34:00, Xi Han wrote:
> > On 2015/04/22 21:15:33, Devlin wrote:
> > > Forward declaration?
> > 
> > We use scoped_ptr<net::URLFetcher> fetcher_, so we need this include.
> 
> scoped_ptr needs only a forward declaration.

It seems yes, I got a wrong understanding for a long time.

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:39: bool operator<(const
UserScriptRenderInfo& other) {
On 2015/04/22 23:20:50, Devlin wrote:
> On 2015/04/22 22:34:00, Xi Han wrote:
> > On 2015/04/22 21:15:33, Devlin wrote:
> > > Why do we need this?
> > 
> > We store the info objects in a map, so it requires to overload the "<"
> operator.
> 
> Only the key type of a std::map requires the operator<, not the value type.

You are right, removed.

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:79: const std::set<int>&
added_script_ids) {
On 2015/04/22 23:20:50, Devlin wrote:
> On 2015/04/22 22:34:00, Xi Han wrote:
> > On 2015/04/22 21:15:33, Devlin wrote:
> > > Let's make this LoadScriptsAndSerialize, and I still think I'd prefer it
to
> > take
> > > a callback (so that OnScriptsLoaded() can be private again).
> > 
> > A compromise is we have to declare the callback type as a public type of
> > UserScriptLoader, since there are some functions in the anonymous namespace
> > refers to it.
> 
> I don't understand.  Why?
> 
> LoadUserScriptsAndSerialize(Callback on_complete) {
>   callback_ = on_complete;
>   FetchStuff();
> }
> 
> OnFetchComplete() {
>   SerializeOnFileThread(callback_);
>   callback_.Reset();
> }
> 
> SerializeOnFileThread(Callback c) {
>   Serialize();
>   c.Run();
> }

I removed the declaration of LoadScriptsCallback from this file, and declare it
in the UserScriptLoader, because the private LoadScripts() needs to know this
type as well:
virtual void LoadScripts(scoped_ptr<UserScriptList> user_scripts,
                         const std::set<HostID>& changed_hosts,
                         const std::set<int>& added_script_ids,
                         LoadScriptsCallback callback) = 0;

However, SerializeOnFileThread(..., UserScriptLoader::LoadScriptsCallback) (in
this file and LoadScriptsOnFileThread(...) in the ExtensionUserScriptLoader)
also needs UserScriptLoader::LoadScriptsCallback in the anonymous namespace. So
I have to declare LoadScriptsCallback as public.

Does it sound good to you?

[Devlin, 2015-04-23 16:34:38]

Also, please note for comments like preferring forward declarations over
includes, or removing extraneous includes, they can generally be applied to the
full CL, so please double-check for other instances of the same. :)

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/600001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:79: const std::set<int>&
added_script_ids) {
On 2015/04/23 15:01:55, Xi Han wrote:
> On 2015/04/22 23:20:50, Devlin wrote:
> > On 2015/04/22 22:34:00, Xi Han wrote:
> > > On 2015/04/22 21:15:33, Devlin wrote:
> > > > Let's make this LoadScriptsAndSerialize, and I still think I'd prefer it
> to
> > > take
> > > > a callback (so that OnScriptsLoaded() can be private again).
> > > 
> > > A compromise is we have to declare the callback type as a public type of
> > > UserScriptLoader, since there are some functions in the anonymous
namespace
> > > refers to it.
> > 
> > I don't understand.  Why?
> > 
> > LoadUserScriptsAndSerialize(Callback on_complete) {
> >   callback_ = on_complete;
> >   FetchStuff();
> > }
> > 
> > OnFetchComplete() {
> >   SerializeOnFileThread(callback_);
> >   callback_.Reset();
> > }
> > 
> > SerializeOnFileThread(Callback c) {
> >   Serialize();
> >   c.Run();
> > }
> 
> I removed the declaration of LoadScriptsCallback from this file, and declare
it
> in the UserScriptLoader, because the private LoadScripts() needs to know this
> type as well:
> virtual void LoadScripts(scoped_ptr<UserScriptList> user_scripts,
>                          const std::set<HostID>& changed_hosts,
>                          const std::set<int>& added_script_ids,
>                          LoadScriptsCallback callback) = 0;
> 
> However, SerializeOnFileThread(..., UserScriptLoader::LoadScriptsCallback) (in
> this file and LoadScriptsOnFileThread(...) in the ExtensionUserScriptLoader)
> also needs UserScriptLoader::LoadScriptsCallback in the anonymous namespace.
So
> I have to declare LoadScriptsCallback as public.
> 
> Does it sound good to you?

Yeah, there's nothing wrong with having the typedef be public.

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/use...
File extensions/browser/user_script_loader.cc (left):

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/use...
extensions/browser/user_script_loader.cc:37: UserScriptLoader::SubstitutionMap*
GetLocalizationMessages(
I trust all of these are just copy-pasted, right?  (Plus a few c++11-style
updates?)

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/use...
File extensions/browser/user_script_loader.h (right):

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/use...
extensions/browser/user_script_loader.h:43: using PathAndDefaultLocale =
std::pair<base::FilePath, std::string>;
Do we need these typedefs in this file?  I think all three
(PathAndDefaultLocale, HostsInfo, and SubstitutionMap) are only used by the
ExtensionUserScriptLoader.

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:8: #include "base/pickle.h"
Please double check your includes.

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:14: namespace {
newline after namespace.

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:42: user_scripts_cache_(nullptr)
{
scoped ptr has a default constructor to initialize to null, so you don't need
this.

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:145: callback_));
reset |callback_| here after this.

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.h (right):

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.h:9: #include
"extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.h"
Forward declaration.

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.h:36: // Called when every single
URL fetch is done. It also traces whether all the
"when every single URL fetch is done" is ambiguous (it could also mean when all
fetches are done).  Let's say "Called at the end of each fetch, tracking whether
all fetches are done."  And you can omit the part about it calling
OnWebUIURLFetchComplete (that's too much info for the comment).

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.h:62: LoadScriptsCallback
callback_;
Let's call this scripts_loaded_callback_.

[Xi Han, 2015-04-23 17:50:40]

PTAL, thanks!

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/use...
File extensions/browser/user_script_loader.cc (left):

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/use...
extensions/browser/user_script_loader.cc:37: UserScriptLoader::SubstitutionMap*
GetLocalizationMessages(
On 2015/04/23 16:34:38, Devlin wrote:
> I trust all of these are just copy-pasted, right?  (Plus a few c++11-style
> updates?)

Yes, totally mechanical movement of code.

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/use...
File extensions/browser/user_script_loader.h (right):

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/use...
extensions/browser/user_script_loader.h:43: using PathAndDefaultLocale =
std::pair<base::FilePath, std::string>;
On 2015/04/23 16:34:38, Devlin wrote:
> Do we need these typedefs in this file?  I think all three
> (PathAndDefaultLocale, HostsInfo, and SubstitutionMap) are only used by the
> ExtensionUserScriptLoader.

Good catch, moved.

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:8: #include "base/pickle.h"
On 2015/04/23 16:34:38, Devlin wrote:
> Please double check your includes.

Removed two unnecessary ones.

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:14: namespace {
On 2015/04/23 16:34:38, Devlin wrote:
> newline after namespace.

Done.

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:42: user_scripts_cache_(nullptr)
{
On 2015/04/23 16:34:38, Devlin wrote:
> scoped ptr has a default constructor to initialize to null, so you don't need
> this.

Got it, thanks.

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:145: callback_));
On 2015/04/23 16:34:38, Devlin wrote:
> reset |callback_| here after this.

Done.

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.h (right):

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.h:9: #include
"extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.h"
On 2015/04/23 16:34:38, Devlin wrote:
> Forward declaration.

Done.

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.h:36: // Called when every single
URL fetch is done. It also traces whether all the
On 2015/04/23 16:34:38, Devlin wrote:
> "when every single URL fetch is done" is ambiguous (it could also mean when
all
> fetches are done).  Let's say "Called at the end of each fetch, tracking
whether
> all fetches are done."  And you can omit the part about it calling
> OnWebUIURLFetchComplete (that's too much info for the comment).

Sounds good:)

https://codereview.chromium.org/1056533002/diff/700001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.h:62: LoadScriptsCallback
callback_;
On 2015/04/23 16:34:38, Devlin wrote:
> Let's call this scripts_loaded_callback_.

Done.

[Devlin, 2015-04-24 17:51:00]

LGTM with nits

https://codereview.chromium.org/1056533002/diff/720001/extensions/browser/ext...
File extensions/browser/extension_user_script_loader.h (right):

https://codereview.chromium.org/1056533002/diff/720001/extensions/browser/ext...
extensions/browser/extension_user_script_loader.h:25: using PathAndDefaultLocale
= std::pair<base::FilePath, std::string>;
Why do these need to be public?  And can't SubstitutionMap be in the .cc file in
the anonymous namespace?

https://codereview.chromium.org/1056533002/diff/720001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.cc
(right):

https://codereview.chromium.org/1056533002/diff/720001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.cc:8: #include
"content/public/common/url_fetcher.h"
Why is this include needed?

https://codereview.chromium.org/1056533002/diff/720001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.cc:47: auto
callback_cache = callback_;
A quick comment on why we do this would be good.
// We cache the callback and reset it so that any references stored within it
are destroyed at the end of the method.

https://codereview.chromium.org/1056533002/diff/720001/extensions/browser/use...
File extensions/browser/user_script_loader.h (right):

https://codereview.chromium.org/1056533002/diff/720001/extensions/browser/use...
extensions/browser/user_script_loader.h:32: class ContentVerifier;
This is no longer needed.

https://codereview.chromium.org/1056533002/diff/720001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/720001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:72: complete_fetchers_ = 0;
It might be better to DCHECK_EQ(0u, complete_fetchers), since we want to
guarantee that we never have multiple LoadScripts calls running, and we reset it
to 0 in the OnComplete method.

[Xi Han, 2015-04-24 18:28:54]

https://codereview.chromium.org/1056533002/diff/720001/extensions/browser/ext...
File extensions/browser/extension_user_script_loader.h (right):

https://codereview.chromium.org/1056533002/diff/720001/extensions/browser/ext...
extensions/browser/extension_user_script_loader.h:25: using PathAndDefaultLocale
= std::pair<base::FilePath, std::string>;
On 2015/04/24 17:50:59, Devlin wrote:
> Why do these need to be public?  And can't SubstitutionMap be in the .cc file
in
> the anonymous namespace?

There are several functions in the anonymous namespace of .cc file use
HostsInfo, so we have to declare HostsInfo and PathAndDefaultLocale as public.
Yes, we should definitely move SubstitutionMap to the .cc file.

https://codereview.chromium.org/1056533002/diff/720001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.cc
(right):

https://codereview.chromium.org/1056533002/diff/720001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.cc:8: #include
"content/public/common/url_fetcher.h"
On 2015/04/24 17:50:59, Devlin wrote:
> Why is this include needed?

Removed.

https://codereview.chromium.org/1056533002/diff/720001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_ui/web_ui_url_fetcher.cc:47: auto
callback_cache = callback_;
On 2015/04/24 17:50:59, Devlin wrote:
> A quick comment on why we do this would be good.
> // We cache the callback and reset it so that any references stored within it
> are destroyed at the end of the method.

Done.

https://codereview.chromium.org/1056533002/diff/720001/extensions/browser/use...
File extensions/browser/user_script_loader.h (right):

https://codereview.chromium.org/1056533002/diff/720001/extensions/browser/use...
extensions/browser/user_script_loader.h:32: class ContentVerifier;
On 2015/04/24 17:50:59, Devlin wrote:
> This is no longer needed.

Move to ExtensionUserScriptLoader.

https://codereview.chromium.org/1056533002/diff/720001/extensions/browser/web...
File extensions/browser/web_ui_user_script_loader.cc (right):

https://codereview.chromium.org/1056533002/diff/720001/extensions/browser/web...
extensions/browser/web_ui_user_script_loader.cc:72: complete_fetchers_ = 0;
On 2015/04/24 17:50:59, Devlin wrote:
> It might be better to DCHECK_EQ(0u, complete_fetchers), since we want to
> guarantee that we never have multiple LoadScripts calls running, and we reset
it
> to 0 in the OnComplete method.

Updated.

[Xi Han, 2015-04-24 18:31:32]

The CQ bit was checked by hanxi@chromium.org

[Xi Han, 2015-04-24 18:31:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, xiyuan@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1056533002/#ps740001
(title: "nits.")

[commit-bot: I haz the power, 2015-04-24 18:32:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1056533002/740001

[commit-bot: I haz the power, 2015-04-24 19:22:03]

Committed patchset #16 (id:740001)

[commit-bot: I haz the power, 2015-04-24 19:22:57]

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