Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
959413003:
Implement <webview>.addContentScript/removeContentScript API [1] (Closed)
|
Created:
5 years, 9 months ago by Xi Han Modified:
5 years, 8 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement <webview>.addContentScript/removeContentScript API [1].
This patch includes the changes that enables <webview>.addContentScript/removeContentScript API work on extensions.
This is the first 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)
TBR=asvitkine@chromium.org
BUG=461052
Committed: https://crrev.com/05a02505462f29d067288ec9e5143c2daad5be78
Cr-Commit-Position: refs/heads/master@{#325492}
Patch Set 1 : Can compile and no crash on extensions. #
Total comments: 12
Patch Set 2 : Fady's comments. #Patch Set 3 : API works on extensions. #Patch Set 4 : Add a test. #
Total comments: 9
Patch Set 5 : Make the API work before the first navigation of guest. #
Total comments: 22
Patch Set 6 : Revert changes in ExtensionSystems and update WebViewContentScriptManager. #
Total comments: 2
Patch Set 7 : Make the sync_IPC handled in IO thread. #
Total comments: 42
Patch Set 8 : Plumb in IsIsolatedGuest status to render, some refactoring and rebase. #Patch Set 9 : Fix a bug and add a test. #Patch Set 10 : Rebase. #Patch Set 11 : Add comments. #
Total comments: 5
Patch Set 12 : Remove plumbing of isolated guest status to renderer. #Patch Set 13 : Rebase. #
Total comments: 29
Patch Set 14 : Devlin's comments and fix the incognito mode issue. #
Total comments: 9
Patch Set 15 : Add WebContentObserver for embedder's webcontents. #
Total comments: 7
Patch Set 16 : nits #Patch Set 17 : Add a test for the lifetime of content scripts. #
Total comments: 102
Patch Set 18 : Delvin's comments. #
Total comments: 29
Patch Set 19 : Another round of comments. #
Total comments: 2
Patch Set 20 : nits. #Patch Set 21 : Update allowed_everywhere for webUI. #
Total comments: 2
Patch Set 22 : Clean up in web_view_internal_api.cc. #
Total comments: 6
Patch Set 23 : Update comments #
Total comments: 12
Patch Set 24 : #
Total comments: 1
Messages
Total messages: 80 (27 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
hanxi@chromium.org changed reviewers: + fsamuel@chromium.org
Hi Fady: I would like you to take a look this patch first. This would be the first step on <webview> sides of changes. Thanks!
https://codereview.chromium.org/959413003/diff/60001/extensions/browser/api/w... File extensions/browser/api/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/60001/extensions/browser/api/w... extensions/browser/api/web_view/web_view_internal_api.cc:94: true /* canExecuteScriptEverywhere*/)); Use unix_hacker style. https://codereview.chromium.org/959413003/diff/60001/extensions/browser/api/w... extensions/browser/api/web_view/web_view_internal_api.cc:162: // include_globs: Remind me what these are? https://codereview.chromium.org/959413003/diff/60001/extensions/browser/api/w... extensions/browser/api/web_view/web_view_internal_api.cc:170: // exclude_globs: Same here. What is this? https://codereview.chromium.org/959413003/diff/60001/extensions/browser/guest... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/959413003/diff/60001/extensions/browser/guest... extensions/browser/guest_view/web_view/web_view_guest.cc:64: Remove this extra line. https://codereview.chromium.org/959413003/diff/60001/extensions/browser/guest... extensions/browser/guest_view/web_view/web_view_guest.cc:648: bool WebViewGuest::IsHostIDEmpty() { What does this mean? https://codereview.chromium.org/959413003/diff/60001/extensions/browser/guest... extensions/browser/guest_view/web_view/web_view_guest.cc:670: master_->AddScript(script); Does this plumbing work now?
Patchset #3 (id:100001) has been deleted
Hi Fady, this patch worked on extension webview. Haven't added tests yet, but I would like you to see these major changes first. Thanks! https://codereview.chromium.org/959413003/diff/60001/extensions/browser/api/w... File extensions/browser/api/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/60001/extensions/browser/api/w... extensions/browser/api/web_view/web_view_internal_api.cc:94: true /* canExecuteScriptEverywhere*/)); On 2015/03/04 02:02:10, Fady Samuel wrote: > Use unix_hacker style. Done. https://codereview.chromium.org/959413003/diff/60001/extensions/browser/api/w... extensions/browser/api/web_view/web_view_internal_api.cc:162: // include_globs: On 2015/03/04 02:02:10, Fady Samuel wrote: > Remind me what these are? Include_globs can be used to limit which pages are affected after "matches". Only the urls that match the pattern defined in "matches" and "include_globs" will be affected. https://developer.chrome.com/extensions/content_scripts#match-patterns-globs https://codereview.chromium.org/959413003/diff/60001/extensions/browser/api/w... extensions/browser/api/web_view/web_view_internal_api.cc:170: // exclude_globs: On 2015/03/04 02:02:10, Fady Samuel wrote: > Same here. What is this? Applied after "matches" to exclude URLs that match this glob. https://codereview.chromium.org/959413003/diff/60001/extensions/browser/guest... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/959413003/diff/60001/extensions/browser/guest... extensions/browser/guest_view/web_view/web_view_guest.cc:64: On 2015/03/04 02:02:10, Fady Samuel wrote: > Remove this extra line. Done. https://codereview.chromium.org/959413003/diff/60001/extensions/browser/guest... extensions/browser/guest_view/web_view/web_view_guest.cc:648: bool WebViewGuest::IsHostIDEmpty() { On 2015/03/04 02:02:10, Fady Samuel wrote: > What does this mean? This function returns whether the host_id_ is nullptr. I remove it for now and update the get function of host_id_. https://codereview.chromium.org/959413003/diff/60001/extensions/browser/guest... extensions/browser/guest_view/web_view/web_view_guest.cc:670: master_->AddScript(script); On 2015/03/04 02:02:10, Fady Samuel wrote: > Does this plumbing work now? For webUI, since webUI needs a different way to load user script, it doesn't work for sure. The support for webUI will be in a different patch. For extensions, it has worked:)
Patchset #4 (id:140001) has been deleted
A test is added for extension webview. Please take a look, thanks!
https://codereview.chromium.org/959413003/diff/160001/extensions/browser/api/... File extensions/browser/api/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/160001/extensions/browser/api/... extensions/browser/api/web_view/web_view_internal_api.cc:71: void InitUserScript(UserScript* script, extensions::WebViewGuest* guest) { This is a lot of code that isn't basic binding glue, which just grows this file a lot (it's big, even too big as it is). https://codereview.chromium.org/959413003/diff/160001/extensions/browser/api/... extensions/browser/api/web_view/web_view_internal_api.cc:319: guest->set_host_id(GenerateHostID(extension(), render_view_host())); Move this into AddContentScripts? https://codereview.chromium.org/959413003/diff/160001/extensions/browser/api/... extensions/browser/api/web_view/web_view_internal_api.cc:326: guest->AddContentScripts(name, script); There's too much magic going on at this layer. This code is just a glue layer for bindings. Most of the logic for operating on the function call should be in WebViewGuest. https://codereview.chromium.org/959413003/diff/160001/extensions/browser/api/... extensions/browser/api/web_view/web_view_internal_api.cc:349: if (!params->script_name_list) { I'd move this code into WebViewGuest. https://codereview.chromium.org/959413003/diff/160001/extensions/browser/api/... extensions/browser/api/web_view/web_view_internal_api.cc:349: if (!params->script_name_list) { Move this logic into WebViewGuest. https://codereview.chromium.org/959413003/diff/160001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/959413003/diff/160001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:647: void WebViewGuest::AddContentScripts(const std::string& script_name, This name is wrong. It adds a single script. Really I think we should simply move all of this code to web_view_content_scripts_helper https://codereview.chromium.org/959413003/diff/160001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:668: void WebViewGuest::RemoveContentScripts(const std::string& script_name) { This name is wrong. It removes a single script. Really I think we should simply move all of this code to web_view_content_scripts_helper
https://codereview.chromium.org/959413003/diff/160001/extensions/browser/user... File extensions/browser/user_script_loader.cc (left): https://codereview.chromium.org/959413003/diff/160001/extensions/browser/user... extensions/browser/user_script_loader.cc:479: if (process->IsIsolatedGuest()) Are we sure this doesn't break things now? Content scripts from an extension should not be in injected into webview.
Patchset #5 (id:180001) has been deleted
Patchset #5 (id:200001) has been deleted
Patchset #5 (id:220001) has been deleted
Hi Fady, I didn't reply each of your last comments since the code changes a lot and many of them are moved to web_view_content_script_manager.h(.cc). PTAL, thanks! https://codereview.chromium.org/959413003/diff/160001/extensions/browser/user... File extensions/browser/user_script_loader.cc (left): https://codereview.chromium.org/959413003/diff/160001/extensions/browser/user... extensions/browser/user_script_loader.cc:479: if (process->IsIsolatedGuest()) On 2015/03/11 11:50:19, Fady Samuel wrote: > Are we sure this doesn't break things now? Content scripts from an extension > should not be in injected into webview. I think we won't, since host_id is sent along with the IPC ExtensionMsg_UpdateUserScripts. The host_id for content scripts of webview will be a real extension_id or webui_id; while host_id of extension content scripts will be an empty string.
Patchset #5 (id:240001) has been deleted
https://codereview.chromium.org/959413003/diff/260001/chrome/browser/extensio... File chrome/browser/extensions/extension_system_impl.cc (right): https://codereview.chromium.org/959413003/diff/260001/chrome/browser/extensio... chrome/browser/extensions/extension_system_impl.cc:312: web_view_content_script_manager_.reset( Let's not modify extensionsystem https://codereview.chromium.org/959413003/diff/260001/extensions/browser/gues... File extensions/browser/guest_view/guest_view_message_filter.cc (right): https://codereview.chromium.org/959413003/diff/260001/extensions/browser/gues... extensions/browser/guest_view/guest_view_message_filter.cc:43: case GuestViewHostMsg_AttachGuest::ID: Add GuestViewHostMsg_CanExecuteContentScript https://codereview.chromium.org/959413003/diff/260001/extensions/browser/gues... extensions/browser/guest_view/guest_view_message_filter.cc:145: render_view_id, &info); Don't use WebViewRendererState, use GuestViewManager https://codereview.chromium.org/959413003/diff/260001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.cc (right): https://codereview.chromium.org/959413003/diff/260001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:30: content::BrowserContext* browser_context) { DCHECK_CURRENTLY_ON(BrowserThread::UI); https://codereview.chromium.org/959413003/diff/260001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:32: ->web_view_content_script_manager(); Hang this off the BrowserContext instead. See https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser... https://codereview.chromium.org/959413003/diff/260001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:39: const std::map<std::string, UserScript>& scripts) { DCHECK_CURRENTLY_ON(BrowserThread::UI); https://codereview.chromium.org/959413003/diff/260001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:65: std::vector<std::string>* script_name_list) { DCHECK_CURRENTLY_ON(BrowserThread::UI); https://codereview.chromium.org/959413003/diff/260001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:101: int script_id) { DCHECK_CURRENTLY_ON(BrowserThread::UI); https://codereview.chromium.org/959413003/diff/260001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/959413003/diff/260001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:162: RoutingInfoKey key; Give it a constructor: RoutingInfoKey key(routing_id, script_->id()); https://codereview.chromium.org/959413003/diff/260001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:167: RoutingInfoMap::iterator iter = map.find(key); nit: auto https://codereview.chromium.org/959413003/diff/260001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:174: rdv->Send(new GuestViewHostMsg_CanExecuteContentScript( I don't think this is necessary: content::RenderThread::Get()->Send ought to do the trick.
Patchset #6 (id:280001) has been deleted
Patchset #6 (id:300001) has been deleted
Hi Fady: PTAL, thanks:) https://codereview.chromium.org/959413003/diff/260001/chrome/browser/extensio... File chrome/browser/extensions/extension_system_impl.cc (right): https://codereview.chromium.org/959413003/diff/260001/chrome/browser/extensio... chrome/browser/extensions/extension_system_impl.cc:312: web_view_content_script_manager_.reset( On 2015/03/27 20:25:36, Fady Samuel wrote: > Let's not modify extensionsystem Reverted. https://codereview.chromium.org/959413003/diff/260001/extensions/browser/gues... File extensions/browser/guest_view/guest_view_message_filter.cc (right): https://codereview.chromium.org/959413003/diff/260001/extensions/browser/gues... extensions/browser/guest_view/guest_view_message_filter.cc:43: case GuestViewHostMsg_AttachGuest::ID: On 2015/03/27 20:25:36, Fady Samuel wrote: > Add GuestViewHostMsg_CanExecuteContentScript Done. https://codereview.chromium.org/959413003/diff/260001/extensions/browser/gues... extensions/browser/guest_view/guest_view_message_filter.cc:145: render_view_id, &info); On 2015/03/27 20:25:36, Fady Samuel wrote: > Don't use WebViewRendererState, use GuestViewManager Done. https://codereview.chromium.org/959413003/diff/260001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.cc (right): https://codereview.chromium.org/959413003/diff/260001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:30: content::BrowserContext* browser_context) { On 2015/03/27 20:25:36, Fady Samuel wrote: > DCHECK_CURRENTLY_ON(BrowserThread::UI); Done. https://codereview.chromium.org/959413003/diff/260001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:32: ->web_view_content_script_manager(); On 2015/03/27 20:25:36, Fady Samuel wrote: > Hang this off the BrowserContext instead. See > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser... Good idea, done! https://codereview.chromium.org/959413003/diff/260001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:39: const std::map<std::string, UserScript>& scripts) { On 2015/03/27 20:25:36, Fady Samuel wrote: > DCHECK_CURRENTLY_ON(BrowserThread::UI); Done. https://codereview.chromium.org/959413003/diff/260001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:65: std::vector<std::string>* script_name_list) { On 2015/03/27 20:25:36, Fady Samuel wrote: > DCHECK_CURRENTLY_ON(BrowserThread::UI); Done. https://codereview.chromium.org/959413003/diff/260001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:101: int script_id) { On 2015/03/27 20:25:36, Fady Samuel wrote: > DCHECK_CURRENTLY_ON(BrowserThread::UI); Done. https://codereview.chromium.org/959413003/diff/260001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/959413003/diff/260001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:162: RoutingInfoKey key; On 2015/03/27 20:25:36, Fady Samuel wrote: > Give it a constructor: > > RoutingInfoKey key(routing_id, script_->id()); Done. https://codereview.chromium.org/959413003/diff/260001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:167: RoutingInfoMap::iterator iter = map.find(key); On 2015/03/27 20:25:36, Fady Samuel wrote: > nit: auto Done. https://codereview.chromium.org/959413003/diff/260001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:174: rdv->Send(new GuestViewHostMsg_CanExecuteContentScript( On 2015/03/27 20:25:36, Fady Samuel wrote: > I don't think this is necessary: > > content::RenderThread::Get()->Send ought to do the trick. Done.
Patchset #7 (id:340001) has been deleted
Hi Fady: I update the IPC as discussed offline, PTAL, thanks!
This is AMAZING! I've been playing with this locally and it works beautifully. At this point, I mostly have nits. The only big concern I have is that we do a sync IPC for non-guests too if they navigate to an origin that hits one of our rules. We need to tell a process it's a guest on SendUpdate. https://codereview.chromium.org/959413003/diff/320001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.cc (right): https://codereview.chromium.org/959413003/diff/320001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:17: using GuestContentScriptMap = std::map<GuestMapKey, ContentScriptMap>; member variables instead so information doesn't leak across profiles. https://codereview.chromium.org/959413003/diff/360001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/959413003/diff/360001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_browsertest.cc:1093: IN_PROC_BROWSER_TEST_F(WebViewTest, Shim_TestAddMultiContentScripts) { AddMultipleContentScripts https://codereview.chromium.org/959413003/diff/360001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/360001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:69: const content::RenderViewHost* rvh) { This is unnecessary. You can grab the SiteuRL from the web_contents. web_contents->GetSiteInstance()->GetSiteURL(). https://codereview.chromium.org/959413003/diff/360001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:74: const GURL& url = rvh->GetSiteInstance()->GetSiteURL(); use web_contents->GetSiteInstance() https://codereview.chromium.org/959413003/diff/360001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:437: int guest_view_instance_id = 0; view_instance_id https://codereview.chromium.org/959413003/diff/360001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:444: if (!render_view_host() || !render_view_host()->GetProcess()) { Is this even possible? https://codereview.chromium.org/959413003/diff/360001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:465: int embedder_process_id = render_view_host()->GetProcess()->GetID(); Use GetSenderWebContents()->GetRenderProcessHost()->GetID() https://codereview.chromium.org/959413003/diff/360001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:499: if (!manager) { I don't think this is possible. I think this simply makes the code more confusing. Maybe a DCHECK(manager)? https://codereview.chromium.org/959413003/diff/360001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:504: if (!render_view_host() || !render_view_host()->GetProcess()) { This check also seems unnecessary. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.cc (right): https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:43: int guest_view_instance_id, nit: view_instance_id https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:52: CHECK(master); Why is this a CHECK? Maybe DCHECK? https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:58: std::pair<int, int>(embedder_process_id, guest_view_instance_id); Call this the view_instance_id to match usage elsewhere. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:64: if (iter == guest_content_script_map_.end()) { This code is a bit difficult to read. How about creating a helper AddContentScript(GuestMapKey key, const UserScript& script)? https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:151: std::set<int> WebViewContentScriptManager::GetContentScriptIdSet( nit: GetContentScriptIDSet https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.h (right): https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:39: int guest_view_instance_id, nit: view_instnace_id https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:50: std::vector<std::string>* script_name_list); const std::vector<std::string>& instead since this is not an output parameter. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:55: int guest_view_instance_id, nit: view_instance_id https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_renderer_state.cc (right): https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_renderer_state.cc:117: for (auto& x : web_view_info_map_) { const auto& render_id_info : web_view_info_map_ https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_renderer_state.cc:118: WebViewInfo& info = x.second; const WebViewInfo& https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_renderer_state.cc:131: std::set<int> script_ids) { const std::set<int>&
https://codereview.chromium.org/959413003/diff/360001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/959413003/diff/360001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_browsertest.cc:1111: IN_PROC_BROWSER_TEST_F(WebViewTest, Shim_TestAddAndRemoveContentScripts) { A NewWindow API test would be awesome too!
https://codereview.chromium.org/959413003/diff/360001/extensions/browser/user... File extensions/browser/user_script_loader.cc (right): https://codereview.chromium.org/959413003/diff/360001/extensions/browser/user... extensions/browser/user_script_loader.cc:450: SendUpdate(i.GetCurrentValue(), shared_memory_.get(), changed_hosts_); SendUpdate should contain a flag indicating whether the process is a web_view.
Patchset #8 (id:380001) has been deleted
PTAL, thanks! https://codereview.chromium.org/959413003/diff/320001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.cc (right): https://codereview.chromium.org/959413003/diff/320001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:17: using GuestContentScriptMap = std::map<GuestMapKey, ContentScriptMap>; On 2015/03/31 23:40:11, Fady Samuel wrote: > member variables instead so information doesn't leak across profiles. Has updated since last patch:) https://codereview.chromium.org/959413003/diff/360001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/959413003/diff/360001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_browsertest.cc:1093: IN_PROC_BROWSER_TEST_F(WebViewTest, Shim_TestAddMultiContentScripts) { On 2015/03/31 23:40:11, Fady Samuel wrote: > AddMultipleContentScripts Done. https://codereview.chromium.org/959413003/diff/360001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_browsertest.cc:1111: IN_PROC_BROWSER_TEST_F(WebViewTest, Shim_TestAddAndRemoveContentScripts) { On 2015/03/31 23:41:12, Fady Samuel wrote: > A NewWindow API test would be awesome too! Added. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/360001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:69: const content::RenderViewHost* rvh) { On 2015/03/31 23:40:12, Fady Samuel wrote: > This is unnecessary. You can grab the SiteuRL from the web_contents. > > web_contents->GetSiteInstance()->GetSiteURL(). That is great. Updated. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:74: const GURL& url = rvh->GetSiteInstance()->GetSiteURL(); On 2015/03/31 23:40:11, Fady Samuel wrote: > use web_contents->GetSiteInstance() Done. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:437: int guest_view_instance_id = 0; On 2015/03/31 23:40:12, Fady Samuel wrote: > view_instance_id Done. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:444: if (!render_view_host() || !render_view_host()->GetProcess()) { On 2015/03/31 23:40:11, Fady Samuel wrote: > Is this even possible? Removed. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:465: int embedder_process_id = render_view_host()->GetProcess()->GetID(); On 2015/03/31 23:40:11, Fady Samuel wrote: > Use GetSenderWebContents()->GetRenderProcessHost()->GetID() Done. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:499: if (!manager) { On 2015/03/31 23:40:11, Fady Samuel wrote: > I don't think this is possible. I think this simply makes the code more > confusing. > > Maybe a DCHECK(manager)? Done. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:504: if (!render_view_host() || !render_view_host()->GetProcess()) { On 2015/03/31 23:40:11, Fady Samuel wrote: > This check also seems unnecessary. Done. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.cc (right): https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:43: int guest_view_instance_id, On 2015/03/31 23:40:12, Fady Samuel wrote: > nit: view_instance_id Done. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:52: CHECK(master); On 2015/03/31 23:40:12, Fady Samuel wrote: > Why is this a CHECK? Maybe DCHECK? Done. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:58: std::pair<int, int>(embedder_process_id, guest_view_instance_id); On 2015/03/31 23:40:12, Fady Samuel wrote: > Call this the view_instance_id to match usage elsewhere. Done. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:64: if (iter == guest_content_script_map_.end()) { On 2015/03/31 23:40:12, Fady Samuel wrote: > This code is a bit difficult to read. How about creating a helper > AddContentScript(GuestMapKey key, const UserScript& script)? Some changes are made to simply the for loop here, hopefully it improves the readability of the code. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:151: std::set<int> WebViewContentScriptManager::GetContentScriptIdSet( On 2015/03/31 23:40:12, Fady Samuel wrote: > nit: GetContentScriptIDSet Done. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.h (right): https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:39: int guest_view_instance_id, On 2015/03/31 23:40:12, Fady Samuel wrote: > nit: view_instnace_id Done. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:50: std::vector<std::string>* script_name_list); On 2015/03/31 23:40:12, Fady Samuel wrote: > const std::vector<std::string>& instead since this is not an output parameter. Done. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:55: int guest_view_instance_id, On 2015/03/31 23:40:12, Fady Samuel wrote: > nit: view_instance_id Done. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_renderer_state.cc (right): https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_renderer_state.cc:117: for (auto& x : web_view_info_map_) { On 2015/03/31 23:40:12, Fady Samuel wrote: > const auto& render_id_info : web_view_info_map_ It cannot be const, since we insert ids to the info.content_script_ids. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_renderer_state.cc:118: WebViewInfo& info = x.second; On 2015/03/31 23:40:12, Fady Samuel wrote: > const WebViewInfo& Same as above. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_renderer_state.cc:131: std::set<int> script_ids) { On 2015/03/31 23:40:12, Fady Samuel wrote: > const std::set<int>& Done. https://codereview.chromium.org/959413003/diff/360001/extensions/browser/user... File extensions/browser/user_script_loader.cc (right): https://codereview.chromium.org/959413003/diff/360001/extensions/browser/user... extensions/browser/user_script_loader.cc:450: SendUpdate(i.GetCurrentValue(), shared_memory_.get(), changed_hosts_); On 2015/04/01 00:00:32, Fady Samuel wrote: > SendUpdate should contain a flag indicating whether the process is a web_view. Done.
Patchset #8 (id:400001) has been deleted
Patchset #9 (id:440001) has been deleted
Patchset #11 (id:500001) has been deleted
Hi Fady: I fixed a bug in my previous patch and add more tests to this CL, PTAL. Thanks!
The more I think about it the more I believe a guest should not know it's a guest. I prefer your previous patch. lgtm to patchset 10 after devlin is happy with it. https://codereview.chromium.org/959413003/diff/520001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/959413003/diff/520001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:877: if (manager) { Why would we not have a manager? I think make it DCHECK instead. https://codereview.chromium.org/959413003/diff/520001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_renderer_state.cc (right): https://codereview.chromium.org/959413003/diff/520001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_renderer_state.cc:122: for (int id : script_ids) { nit: braces not necessary. https://codereview.chromium.org/959413003/diff/520001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_renderer_state.cc:139: info.instance_id == view_instance_id) { nit: braces not necessary. https://codereview.chromium.org/959413003/diff/520001/extensions/common/exten... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/959413003/diff/520001/extensions/common/exten... extensions/common/extension_messages.h:460: // |is_to_guest_render_process| indicates whether the IPC is sent to a guest nit: is_guest_process is less wordy https://codereview.chromium.org/959413003/diff/520001/extensions/renderer/use... File extensions/renderer/user_script_set_manager.h (right): https://codereview.chromium.org/959413003/diff/520001/extensions/renderer/use... extensions/renderer/user_script_set_manager.h:105: bool is_in_guest_render_process_; is_guest_process_ is less wordy.
hanxi@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Hi Devlin: This is the first part of this API, and it following patch is: https://codereview.chromium.org/1056533002/ We target to make this API work for M44. Please review all changes.Thanks.
A few high-level comments first. https://codereview.chromium.org/959413003/diff/560001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/560001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:68: HostID GenerateHostID(const extensions::Extension* extension, Function comments (also on 81) https://codereview.chromium.org/959413003/diff/560001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:81: bool Parse(const ContentScriptDetails& script_value, "Parse" is pretty vague. You should include what you are parsing. https://codereview.chromium.org/959413003/diff/560001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:81: bool Parse(const ContentScriptDetails& script_value, Is there no way we can share this functionality with the manifest handler? https://codereview.chromium.org/959413003/diff/560001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.h (right): https://codereview.chromium.org/959413003/diff/560001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.h:132: class WebViewInternalAddContentScriptsFunction : public AsyncExtensionFunction { AsyncExtensionFunction is dead. Long live UIThreadExtensionFunction. https://codereview.chromium.org/959413003/diff/560001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.h (right): https://codereview.chromium.org/959413003/diff/560001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:26: class WebViewContentScriptManager : public base::SupportsUserData::Data { Do we really need this class? Couldn't adding/removing the content scripts from the UserScriptMaster and updating the RendererState be done by the api function? https://codereview.chromium.org/959413003/diff/560001/extensions/common/api/w... File extensions/common/api/web_view_internal.json (right): https://codereview.chromium.org/959413003/diff/560001/extensions/common/api/w... extensions/common/api/web_view_internal.json:98: "id": "RunLocation", Why not reference the RunLocation in extension_types? https://codereview.chromium.org/959413003/diff/560001/extensions/common/api/w... extensions/common/api/web_view_internal.json:277: "decsiption": "The name of a content script that will be removed." typo https://codereview.chromium.org/959413003/diff/560001/extensions/common/guest... File extensions/common/guest_view/guest_view_messages.h (right): https://codereview.chromium.org/959413003/diff/560001/extensions/common/guest... extensions/common/guest_view/guest_view_messages.h:63: IPC_SYNC_MESSAGE_CONTROL2_1(GuestViewHostMsg_CanExecuteContentScript, nit: Let's put sync in the name (i.e., CanExecuteContentScriptSync) https://codereview.chromium.org/959413003/diff/560001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/959413003/diff/560001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:57: LAZY_INSTANCE_INITIALIZER; Is there a way we can clear this map? Probably not a huge deal, but worth doing, or documenting why it's not vital. https://codereview.chromium.org/959413003/diff/560001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:178: new GuestViewHostMsg_CanExecuteContentScript(routing_id, script_->id(), This still makes me a little sad, and it'd be better with a big comment along the lines of // Send a SYNC IPC message to the browser to check if this is allowed. This is // not ideal, but is mitigated by the fact that this is only done for webviews, // and then only once per host. // TODO(hanxi): Find a more efficient way to do this.
Patchset #14 (id:580001) has been deleted
Devlin:PTAL,thanks. https://codereview.chromium.org/959413003/diff/560001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/560001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:68: HostID GenerateHostID(const extensions::Extension* extension, On 2015/04/08 17:50:56, Devlin wrote: > Function comments (also on 81) Changed to GenerateHostIDFromEmbedder. https://codereview.chromium.org/959413003/diff/560001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:81: bool Parse(const ContentScriptDetails& script_value, On 2015/04/08 17:50:56, Devlin wrote: > "Parse" is pretty vague. You should include what you are parsing. Rename to ParseContentScript(...). https://codereview.chromium.org/959413003/diff/560001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:81: bool Parse(const ContentScriptDetails& script_value, On 2015/04/08 17:50:56, Devlin wrote: > Is there no way we can share this functionality with the manifest handler? I would like to, but the biggest difference is how the data is stored. In ContentScriptsHandler, the data is stored in a DictionaryValue object. https://codereview.chromium.org/959413003/diff/560001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.h (right): https://codereview.chromium.org/959413003/diff/560001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.h:132: class WebViewInternalAddContentScriptsFunction : public AsyncExtensionFunction { On 2015/04/08 17:50:56, Devlin wrote: > AsyncExtensionFunction is dead. Long live UIThreadExtensionFunction. Updated here, since this API doesn't send an Async response. https://codereview.chromium.org/959413003/diff/560001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.h (right): https://codereview.chromium.org/959413003/diff/560001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:26: class WebViewContentScriptManager : public base::SupportsUserData::Data { On 2015/04/08 17:50:56, Devlin wrote: > Do we really need this class? Couldn't adding/removing the content scripts from > the UserScriptMaster and updating the RendererState be done by the api function? It is better to have this class to keep other classes clean. The WebViewInternalAPI is just a glue level, which is responsible for converting the data from javascript to c++, so it shouldn't know too much about how to handle these data. Also I try to avoid putting webview-specific code to UserScriptMaster. https://codereview.chromium.org/959413003/diff/560001/extensions/common/api/w... File extensions/common/api/web_view_internal.json (right): https://codereview.chromium.org/959413003/diff/560001/extensions/common/api/w... extensions/common/api/web_view_internal.json:98: "id": "RunLocation", On 2015/04/08 17:50:56, Devlin wrote: > Why not reference the RunLocation in extension_types? There is a bug in the compiler of json files after making it refer to extensionTypes.RunAt. The namespace isn't properly added in the web_view_internal.cc file. We need to fix the bug first. Maybe the easiest way is to just define the RunLocation here? https://codereview.chromium.org/959413003/diff/560001/extensions/common/api/w... extensions/common/api/web_view_internal.json:277: "decsiption": "The name of a content script that will be removed." On 2015/04/08 17:50:56, Devlin wrote: > typo Done. https://codereview.chromium.org/959413003/diff/560001/extensions/common/guest... File extensions/common/guest_view/guest_view_messages.h (right): https://codereview.chromium.org/959413003/diff/560001/extensions/common/guest... extensions/common/guest_view/guest_view_messages.h:63: IPC_SYNC_MESSAGE_CONTROL2_1(GuestViewHostMsg_CanExecuteContentScript, On 2015/04/08 17:50:56, Devlin wrote: > nit: Let's put sync in the name (i.e., CanExecuteContentScriptSync) Done. https://codereview.chromium.org/959413003/diff/560001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/959413003/diff/560001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:57: LAZY_INSTANCE_INITIALIZER; On 2015/04/08 17:50:56, Devlin wrote: > Is there a way we can clear this map? Probably not a huge deal, but worth > doing, or documenting why it's not vital. I have the same thoughts, but it is difficult to put the map in a place that we can add entries here, and remove entries somewhere else. Add some comments for why it is ok not update it. https://codereview.chromium.org/959413003/diff/560001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:178: new GuestViewHostMsg_CanExecuteContentScript(routing_id, script_->id(), On 2015/04/08 17:50:56, Devlin wrote: > This still makes me a little sad, and it'd be better with a big comment along > the lines of > // Send a SYNC IPC message to the browser to check if this is allowed. This is > // not ideal, but is mitigated by the fact that this is only done for webviews, > // and then only once per host. > // TODO(hanxi): Find a more efficient way to do this. Done.
I just realized we need to do some lifetime management of content scripts here. See GuestViewBase::OwnerContentsObserver for an example of how to do this: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser... https://codereview.chromium.org/959413003/diff/600001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/600001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:470: manager->AddContentScripts(embedder_process_id, view_instance_id, host_id, Maybe pass in GetSenderWebContents() instead? That's the embedder WebContents. We can use that for tracking when the embedder goes away. https://codereview.chromium.org/959413003/diff/600001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:505: manager->RemoveContentScripts(embedder_process_id, view_instance_id, Take in the embedder WebContents here. https://codereview.chromium.org/959413003/diff/600001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.cc (right): https://codereview.chromium.org/959413003/diff/600001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:42: int embedder_process_id, Take in an embedder_web_contents here. Keep a map from WebContents* to scoped_ptr<WebContentsObserver*>. If the WebContentsObserver sees a main frame navigation or sees the process has went away or it is being destroyed, then call RemoveContentScripts to delete all associated content scripts. https://codereview.chromium.org/959413003/diff/600001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:71: // If a content script has the same name as the new one, removes the old nit: remove https://codereview.chromium.org/959413003/diff/600001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.h (right): https://codereview.chromium.org/959413003/diff/600001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:38: void AddContentScripts(int embedder_process_id, Take in the embedder_web_contents https://codereview.chromium.org/959413003/diff/600001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:47: void RemoveContentScripts(int embedder_process_id, Take in the embedder_web_contents
Patchset #15 (id:620001) has been deleted
Hi Fady, I add the web contents observer for embedder, so we can clean up guests' content scripts after the embedder webcontents is gone and so on. PTAL, thanks.
https://codereview.chromium.org/959413003/diff/560001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/560001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:81: bool Parse(const ContentScriptDetails& script_value, On 2015/04/08 22:10:36, Xi Han wrote: > On 2015/04/08 17:50:56, Devlin wrote: > > Is there no way we can share this functionality with the manifest handler? > > I would like to, but the biggest difference is how the data is stored. In > ContentScriptsHandler, the data is stored in a DictionaryValue object. ContentScriptDetails can be created from a Dictionary, and is stored as a dictionary on the extension function's |params_|. https://codereview.chromium.org/959413003/diff/560001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.h (right): https://codereview.chromium.org/959413003/diff/560001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.h:132: class WebViewInternalAddContentScriptsFunction : public AsyncExtensionFunction { On 2015/04/08 22:10:36, Xi Han wrote: > On 2015/04/08 17:50:56, Devlin wrote: > > AsyncExtensionFunction is dead. Long live UIThreadExtensionFunction. > > Updated here, since this API doesn't send an Async response. FYI, UIThreadExtensionFunction is used for both Sync and Async extension functions. https://codereview.chromium.org/959413003/diff/560001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.h (right): https://codereview.chromium.org/959413003/diff/560001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:26: class WebViewContentScriptManager : public base::SupportsUserData::Data { On 2015/04/08 22:10:36, Xi Han wrote: > On 2015/04/08 17:50:56, Devlin wrote: > > Do we really need this class? Couldn't adding/removing the content scripts > from > > the UserScriptMaster and updating the RendererState be done by the api > function? > > It is better to have this class to keep other classes clean. The > WebViewInternalAPI is just a glue level, which is responsible for converting the > data from javascript to c++, so it shouldn't know too much about how to handle > these data. Also I try to avoid putting webview-specific code to > UserScriptMaster. I'm still not entirely convinced of the usefulness/necessity of the class. Why would something like: - API function adds scripts to renderer state - Renderer state adds content scripts to UserScriptMaster, removes them when guest is removed Not work? My concern is that the lifetime of user scripts is already hard to keep track of, and I don't know if we want to complicate it yet further. I think I'd rather always know that the scripts are owned by the UserScriptMaster. https://codereview.chromium.org/959413003/diff/600001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/600001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:447: error_ = kViewInstanceIdError; RespondNow(Error(x)) sets error_ to x. https://codereview.chromium.org/959413003/diff/640001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.cc (right): https://codereview.chromium.org/959413003/diff/640001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:152: master->RemoveScript(map_iter->second); What happens when a WebUI has two webviews, and adds the same content script to each? Should we really remove it from the master?
Thanks for fixing the lifetime management issue. This still lgtm. I do see Devlin's comment which is a bit concerning, adding content scripts in this API is a bit heavy weight. Maybe in the future, behind the scenes, multiple webviews can share a single UserScript... I don't think we need to change the API for that though. We can simply match rules that effectively do the same thing across webviews. https://codereview.chromium.org/959413003/diff/640001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.cc (right): https://codereview.chromium.org/959413003/diff/640001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:42: return true; I think you should return false here too to allow ForEachGuest to iterate to all other guests. https://codereview.chromium.org/959413003/diff/640001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:152: master->RemoveScript(map_iter->second); On 2015/04/10 16:04:32, Devlin wrote: > What happens when a WebUI has two webviews, and adds the same content script to > each? Should we really remove it from the master? This API doesn't allow two webviews to share a content script rule. If the same script is injected into two webviews they'd be two different UserScripts, I think. Is that correct Xi? https://codereview.chromium.org/959413003/diff/640001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.h (right): https://codereview.chromium.org/959413003/diff/640001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:58: void RemoveContentScripts(content::WebContents* embedder_web_content, Make this private and make OwnerWebContentsObserver a friend of this class.
https://codereview.chromium.org/959413003/diff/560001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.h (right): https://codereview.chromium.org/959413003/diff/560001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:26: class WebViewContentScriptManager : public base::SupportsUserData::Data { On 2015/04/10 16:04:31, Devlin wrote: > On 2015/04/08 22:10:36, Xi Han wrote: > > On 2015/04/08 17:50:56, Devlin wrote: > > > Do we really need this class? Couldn't adding/removing the content scripts > > from > > > the UserScriptMaster and updating the RendererState be done by the api > > function? > > > > It is better to have this class to keep other classes clean. The > > WebViewInternalAPI is just a glue level, which is responsible for converting > the > > data from javascript to c++, so it shouldn't know too much about how to handle > > these data. Also I try to avoid putting webview-specific code to > > UserScriptMaster. > > I'm still not entirely convinced of the usefulness/necessity of the class. Why > would something like: > - API function adds scripts to renderer state > - Renderer state adds content scripts to UserScriptMaster, removes them when > guest is removed > Not work? > > My concern is that the lifetime of user scripts is already hard to keep track > of, and I don't know if we want to complicate it yet further. I think I'd > rather always know that the scripts are owned by the UserScriptMaster. Devlin, the content scripts are tied to the lifetime of the <webview> tag not the guest. <webview> tags can gain and lose guests within their own lifetime. For example, if you remove a webview tag from the DOM, it loses its guest. If you put it back and set the src attribute, it gets a new guest. The guest may be created by another guest via window.open, and it only gets a <webview> as a container on attachment in the new window API: https://developer.chrome.com/apps/tags/webview#type-NewWindow
https://codereview.chromium.org/959413003/diff/560001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.h (right): https://codereview.chromium.org/959413003/diff/560001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:26: class WebViewContentScriptManager : public base::SupportsUserData::Data { On 2015/04/10 16:44:28, Fady Samuel wrote: > Devlin, the content scripts are tied to the lifetime of the <webview> tag not > the guest. <webview> tags can gain and lose guests within their own lifetime. > For example, if you remove a webview tag from the DOM, it loses its guest. If > you put it back and set the src attribute, it gets a new guest. > The guest may be created by another guest via window.open, and it only gets a > <webview> as a container on attachment in the new window API: > > https://developer.chrome.com/apps/tags/webview#type-NewWindow Ah, interesting. So a WebViewInfo is removed when the guest is removed, even though the web view, in fact, lives on, with the same process and view id? Is there a reason we need to remove the WebViewInfo then, as opposed to when the webview is destroyed (do we know when that is)?
There isn't much code changes, but try my best to answer all the questions. https://codereview.chromium.org/959413003/diff/560001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/560001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:81: bool Parse(const ContentScriptDetails& script_value, On 2015/04/10 16:04:31, Devlin wrote: > On 2015/04/08 22:10:36, Xi Han wrote: > > On 2015/04/08 17:50:56, Devlin wrote: > > > Is there no way we can share this functionality with the manifest handler? > > > > I would like to, but the biggest difference is how the data is stored. In > > ContentScriptsHandler, the data is stored in a DictionaryValue object. > > ContentScriptDetails can be created from a Dictionary, and is stored as a > dictionary on the extension function's |params_|. I don't like code duplication too, but the advantage to have webview's implementaion is flexibility. We can change these code when the requirements of this API changes, and ContentScriptsHandler has something that we don't need to consider, like PermissionsData. Personally, I prefer to have the webview-specific parse function. https://codereview.chromium.org/959413003/diff/560001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.h (right): https://codereview.chromium.org/959413003/diff/560001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.h:132: class WebViewInternalAddContentScriptsFunction : public AsyncExtensionFunction { On 2015/04/10 16:04:31, Devlin wrote: > On 2015/04/08 22:10:36, Xi Han wrote: > > On 2015/04/08 17:50:56, Devlin wrote: > > > AsyncExtensionFunction is dead. Long live UIThreadExtensionFunction. > > > > Updated here, since this API doesn't send an Async response. > > FYI, UIThreadExtensionFunction is used for both Sync and Async extension > functions. Acknowledged. https://codereview.chromium.org/959413003/diff/560001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.h (right): https://codereview.chromium.org/959413003/diff/560001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:26: class WebViewContentScriptManager : public base::SupportsUserData::Data { On 2015/04/10 17:38:57, Devlin wrote: > On 2015/04/10 16:44:28, Fady Samuel wrote: > > Devlin, the content scripts are tied to the lifetime of the <webview> tag not > > the guest. <webview> tags can gain and lose guests within their own lifetime. > > For example, if you remove a webview tag from the DOM, it loses its guest. If > > you put it back and set the src attribute, it gets a new guest. > > The guest may be created by another guest via window.open, and it only gets a > > <webview> as a container on attachment in the new window API: > > > > https://developer.chrome.com/apps/tags/webview#type-NewWindow > > Ah, interesting. So a WebViewInfo is removed when the guest is removed, even > though the web view, in fact, lives on, with the same process and view id? Is > there a reason we need to remove the WebViewInfo then, as opposed to when the > webview is destroyed (do we know when that is)? WebViewInfo contains the routing info for a guest and should be accessed in IO thread. In the case that code needs to be run in IO thread, we can access the routing info for the guest. If the guest goes away, its routing info doesn't needed anymore, so we need to remove the WebViewInfo. https://codereview.chromium.org/959413003/diff/600001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/600001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:447: error_ = kViewInstanceIdError; On 2015/04/10 16:04:31, Devlin wrote: > RespondNow(Error(x)) sets error_ to x. Done. https://codereview.chromium.org/959413003/diff/600001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:470: manager->AddContentScripts(embedder_process_id, view_instance_id, host_id, On 2015/04/08 22:40:04, Fady Samuel wrote: > Maybe pass in GetSenderWebContents() instead? That's the embedder WebContents. > We can use that for tracking when the embedder goes away. Sorry, forget to update here. https://codereview.chromium.org/959413003/diff/640001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.cc (right): https://codereview.chromium.org/959413003/diff/640001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:42: return true; On 2015/04/10 16:16:04, Fady Samuel wrote: > I think you should return false here too to allow ForEachGuest to iterate to all > other guests. Done. https://codereview.chromium.org/959413003/diff/640001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:152: master->RemoveScript(map_iter->second); On 2015/04/10 16:16:04, Fady Samuel wrote: > On 2015/04/10 16:04:32, Devlin wrote: > > What happens when a WebUI has two webviews, and adds the same content script > to > > each? Should we really remove it from the master? > > This API doesn't allow two webviews to share a content script rule. If the same > script is injected into two webviews they'd be two different UserScripts, I > think. Is that correct Xi? We will create two different user scripts for them. Currently, we only support adding rules by specifying the entire rule, while don't support adding rule by name. Even for the same name ruleA, we treat <webview1>.ruleA as a different rule as <webview2>.ruleA. https://codereview.chromium.org/959413003/diff/640001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.h (right): https://codereview.chromium.org/959413003/diff/640001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:58: void RemoveContentScripts(content::WebContents* embedder_web_content, On 2015/04/10 16:16:04, Fady Samuel wrote: > Make this private and make OwnerWebContentsObserver a friend of this class. You are absolute right, forget to update here. Updated.
https://codereview.chromium.org/959413003/diff/560001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.h (right): https://codereview.chromium.org/959413003/diff/560001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:26: class WebViewContentScriptManager : public base::SupportsUserData::Data { On 2015/04/10 18:27:06, Xi Han wrote: > > WebViewInfo contains the routing info for a guest and should be accessed in IO > thread. > In the case that code needs to be run in IO thread, we can access the routing > info for > the guest. If the guest goes away, its routing info doesn't needed anymore, so > we need > to remove the WebViewInfo. But that routing info is still valid for the next guest (if any) in that <webview> tag, correct? So if we add another guest in that same tag, then we are just recreating a WebViewInfo that we just destroyed, since the routing info (view id and process id) is linked to the webview tag, not to the guest itself, right? So wouldn't it make sense to remove the WebViewInfo when the webview is destroyed, not when the guest is destroyed? Even more so when we include content scripts (as Fady points out, linked to the <webview>, not the guest) on the WebViewInfo....
On 2015/04/10 18:31:36, Devlin wrote: > https://codereview.chromium.org/959413003/diff/560001/extensions/browser/gues... > File extensions/browser/guest_view/web_view/web_view_content_script_manager.h > (right): > > https://codereview.chromium.org/959413003/diff/560001/extensions/browser/gues... > extensions/browser/guest_view/web_view/web_view_content_script_manager.h:26: > class WebViewContentScriptManager : public base::SupportsUserData::Data { > On 2015/04/10 18:27:06, Xi Han wrote: > > > > WebViewInfo contains the routing info for a guest and should be accessed in IO > > thread. > > In the case that code needs to be run in IO thread, we can access the routing > > info for > > the guest. If the guest goes away, its routing info doesn't needed anymore, so > > we need > > to remove the WebViewInfo. > > But that routing info is still valid for the next guest (if any) in that > <webview> tag, correct? So if we add another guest in that same tag, then we > are just recreating a WebViewInfo that we just destroyed, since the routing info > (view id and process id) is linked to the webview tag, not to the guest itself, > right? So wouldn't it make sense to remove the WebViewInfo when the webview is > destroyed, not when the guest is destroyed? Even more so when we include > content scripts (as Fady points out, linked to the <webview>, not the guest) on > the WebViewInfo.... In the web_view_render_state.h(cc) we use a map to store the WebViewInfo, the key of this map is |guest_process_id, guest_routing_id|. When the guest is destroyed, the guest_routing_id doesn't exist. Even later we create a guest and attached it to the same webview tag, its routing_id will be different. This means we can not use the new |guest_process_id, guest_routing_id| pair to find the previous WebViewInfo.
In the web_view_render_state.h(cc) we use a map to store the WebViewInfo, the key of this map is |guest_process_id, guest_routing_id|. When the guest is destroyed, the guest_routing_id doesn't exist. Even later we create a guest and attached it to the same webview tag, its routing_id will be different. This means we can not use the new |guest_process_id, guest_routing_id| pair to find the previous WebViewInfo.
https://codereview.chromium.org/959413003/diff/680001/chrome/test/data/extens... File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right): https://codereview.chromium.org/959413003/diff/680001/chrome/test/data/extens... chrome/test/data/extensions/platform_apps/web_view/shim/main.js:754: function testAddContentScript() { Describe what the test does in comments (for all) https://codereview.chromium.org/959413003/diff/680001/chrome/test/data/extens... chrome/test/data/extensions/platform_apps/web_view/shim/main.js:878: } else if (data == 'connected_response') { These messages and responses are used enough that they should probably be constants (which would also help with readability). https://codereview.chromium.org/959413003/diff/680001/chrome/test/data/extens... chrome/test/data/extensions/platform_apps/web_view/shim/main.js:884: }, 1000); Why the timeout? https://codereview.chromium.org/959413003/diff/680001/chrome/test/data/extens... chrome/test/data/extensions/platform_apps/web_view/shim/main.js:947: count++; nit: pre-increment https://codereview.chromium.org/959413003/diff/680001/chrome/test/data/extens... chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1108: return; these returns are redundant with the if/else. https://codereview.chromium.org/959413003/diff/680001/chrome/test/data/extens... chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1126: Can we add a test for navigation? https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:82: bool ParseContentScript(const ContentScriptDetails& script_value, Add function comments https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:89: if (matches.size() == 0) { nit: no brackets around single-line ifs https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:95: true /* canExecuteScriptEverywhere */)); Why do we allow these to execute everywhere? https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:142: const std::vector<std::string>& css_files = *(script_value.css.get()); nit: inline css_files https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:158: const std::vector<std::string>& js_files = *(script_value.js.get()); inline https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:173: if (script_value.all_frames) { nit: brackets https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:179: const std::vector<std::string>& include_globs = inline https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:187: const std::vector<std::string>& exclude_globs = inline https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:202: if (content_script_list.size() == 0) nit: Prefer if (content_script_list.empty()) https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:204: for (size_t i = 0; i < content_script_list.size(); ++i) { Why not range-based for loop? https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:211: else nit: no need for the else https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:222: if (!script) Why would we pass in a null script? https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:225: script->set_name(name); Why not set the name in the Parsing process? https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:446: if (!args_->GetInteger(0, &view_instance_id) || !view_instance_id) Why aren't you using the params? https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:460: GenerateHostIDFromEmbedder(extension(), GetSenderWebContents()); cache GetSenderWebContents for use on line 466 https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:464: InitUserScript(&iter.second, iter.first, is_incognito_enabled, host_id); InitUserScript is small enough that let's just inline it here. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:487: if (!args_->GetInteger(0, &view_instance_id) || !view_instance_id) { Same as above, why not use |params|? https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:489: return RespondNow(Error(error_)); RespondNow(Error()) sets the error. So you don't need line 488. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:499: manager->RemoveContentScripts(GetSenderWebContents(), view_instance_id, nit: std::vector<std::string> script_name_list; if (params->script_name_list) script_name_list.swap(*params->script_name_list); manager->RemoveContentScripts(GetSenderWebContents(), view_instance_id, host_id, script_name_list); https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... File extensions/browser/guest_view/guest_view_message_filter.h (right): https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/guest_view_message_filter.h:65: bool* allowed); nit: newline. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.cc (right): https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:25: namespace { nit: This is long enough that there should be newlines around the namespace { } and it should end in } // namespace. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:27: HostID host_id, const & https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:36: if (!manager) We always pass in the same BrowserContext, and we do it from the manager - so how could this fail? That said, we might actually be able to get rid of this. See below. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:50: class WebViewContentScriptManager::OwnerWebContentsObserver Document lifetime. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:81: web_view_content_script_manager_->RemoveContentScripts(web_contents(), Document that this object can be deleted after this line. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:141: key, std::map<std::string, UserScript>())); s/std::map<std::string, UserScript>()/ContentScriptMap() https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:152: master->RemoveScript(map_iter->second); We do an AttemptLoad() in UserScriptLoader after each call to Remove/AddScript(). I think we should expose a batch method on the UserScriptMaster to make this more efficient. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:164: if (observers_map_iter == owner_web_contents_observer_map_.end()) { no reason to cache observers_map_iter, so just inline it in the if. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:167: owner_web_contents_observer_map_[embedder_web_contents] = observer; (See below to make sense of this) observer->add_instance_id(view_instance_id); https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:175: base::Unretained(WebViewRendererState::GetInstance()), document why this unretained is safe. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:216: names_to_delete = script_name_list; this is a potentially expensive copy. Use std::vector::swap instead. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:244: // Step 1: removes content scripts of all the guests embedded. Why not instead store a std::set of view_instance_ids on the OwnerWebContentsObservers, and then pass that in the RemoveContentScripts method. That way, you can just loop over that, instead of having to do multiple lookups. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.h (right): https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:27: class WebViewContentScriptManager : public base::SupportsUserData::Data { Please file a bug and add a TODO here linking to it that we shouldn't have this class managing webview lifetime (and possibly might not need it at all). https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:42: const std::map<std::string, UserScript>& user_scripts); The name is stored on the UserScript, so we don't need to pass in a map here. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:86: content::BrowserContext* browser_context_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_renderer_state.cc (right): https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_renderer_state.cc:122: for (int id : script_ids) { no brackets https://codereview.chromium.org/959413003/diff/680001/extensions/common/api/w... File extensions/common/api/web_view_internal.json (right): https://codereview.chromium.org/959413003/diff/680001/extensions/common/api/w... extensions/common/api/web_view_internal.json:98: "id": "RunLocation", What was the error with trying to use ExtensionTypes' RunLocation? https://codereview.chromium.org/959413003/diff/680001/extensions/renderer/res... File extensions/renderer/resources/guest_view/web_view/web_view_api_methods.js (right): https://codereview.chromium.org/959413003/diff/680001/extensions/renderer/res... extensions/renderer/resources/guest_view/web_view/web_view_api_methods.js:99: var args = $Array.concat([this.viewInstanceId], $Array.slice(arguments)); https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/a... seems to think slicing on arguments is bad. I don't know much more than that, though. https://codereview.chromium.org/959413003/diff/680001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/959413003/diff/680001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:35: this->routing_id = routing_id; no need for "this"
Patchset #18 (id:700001) has been deleted
PTAL, thanks! https://codereview.chromium.org/959413003/diff/680001/chrome/test/data/extens... File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right): https://codereview.chromium.org/959413003/diff/680001/chrome/test/data/extens... chrome/test/data/extensions/platform_apps/web_view/shim/main.js:754: function testAddContentScript() { On 2015/04/13 19:21:46, Devlin wrote: > Describe what the test does in comments (for all) Done. https://codereview.chromium.org/959413003/diff/680001/chrome/test/data/extens... chrome/test/data/extensions/platform_apps/web_view/shim/main.js:878: } else if (data == 'connected_response') { On 2015/04/13 19:21:46, Devlin wrote: > These messages and responses are used enough that they should probably be > constants (which would also help with readability). Done. https://codereview.chromium.org/959413003/diff/680001/chrome/test/data/extens... chrome/test/data/extensions/platform_apps/web_view/shim/main.js:884: }, 1000); On 2015/04/13 19:21:46, Devlin wrote: > Why the timeout? Here we verify that we won't get any message from the first script which is replaced by the second one, since they have the same name. https://codereview.chromium.org/959413003/diff/680001/chrome/test/data/extens... chrome/test/data/extensions/platform_apps/web_view/shim/main.js:947: count++; On 2015/04/13 19:21:46, Devlin wrote: > nit: pre-increment Done. https://codereview.chromium.org/959413003/diff/680001/chrome/test/data/extens... chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1108: return; On 2015/04/13 19:21:46, Devlin wrote: > these returns are redundant with the if/else. removed. https://codereview.chromium.org/959413003/diff/680001/chrome/test/data/extens... chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1126: On 2015/04/13 19:21:46, Devlin wrote: > Can we add a test for navigation? Everytime when we set the src of <webview>, it trigges a navigation. Do you want to have a test for multiple navigation? Or to navigate to a url that shouldn't inject scripts? https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:82: bool ParseContentScript(const ContentScriptDetails& script_value, On 2015/04/13 19:21:46, Devlin wrote: > Add function comments Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:89: if (matches.size() == 0) { On 2015/04/13 19:21:47, Devlin wrote: > nit: no brackets around single-line ifs Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:95: true /* canExecuteScriptEverywhere */)); On 2015/04/13 19:21:47, Devlin wrote: > Why do we allow these to execute everywhere? I checked the implementation in ContentScriptsHandler for extensions, and it looks up whether the extension is in the white list for scripts. Since we don't specify any white list for <webview>, can we assume here should be true? https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:142: const std::vector<std::string>& css_files = *(script_value.css.get()); On 2015/04/13 19:21:47, Devlin wrote: > nit: inline css_files Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:158: const std::vector<std::string>& js_files = *(script_value.js.get()); On 2015/04/13 19:21:47, Devlin wrote: > inline Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:173: if (script_value.all_frames) { On 2015/04/13 19:21:46, Devlin wrote: > nit: brackets Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:179: const std::vector<std::string>& include_globs = On 2015/04/13 19:21:46, Devlin wrote: > inline Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:187: const std::vector<std::string>& exclude_globs = On 2015/04/13 19:21:47, Devlin wrote: > inline Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:202: if (content_script_list.size() == 0) On 2015/04/13 19:21:47, Devlin wrote: > nit: Prefer if (content_script_list.empty()) Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:204: for (size_t i = 0; i < content_script_list.size(); ++i) { On 2015/04/13 19:21:47, Devlin wrote: > Why not range-based for loop? Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:211: else On 2015/04/13 19:21:47, Devlin wrote: > nit: no need for the else Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:222: if (!script) On 2015/04/13 19:21:47, Devlin wrote: > Why would we pass in a null script? That is a good point. Removed the check. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:225: script->set_name(name); On 2015/04/13 19:21:46, Devlin wrote: > Why not set the name in the Parsing process? Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:446: if (!args_->GetInteger(0, &view_instance_id) || !view_instance_id) On 2015/04/13 19:21:47, Devlin wrote: > Why aren't you using the params? We could, updated. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:460: GenerateHostIDFromEmbedder(extension(), GetSenderWebContents()); On 2015/04/13 19:21:47, Devlin wrote: > cache GetSenderWebContents for use on line 466 Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:464: InitUserScript(&iter.second, iter.first, is_incognito_enabled, host_id); On 2015/04/13 19:21:47, Devlin wrote: > InitUserScript is small enough that let's just inline it here. Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:487: if (!args_->GetInteger(0, &view_instance_id) || !view_instance_id) { On 2015/04/13 19:21:47, Devlin wrote: > Same as above, why not use |params|? Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:489: return RespondNow(Error(error_)); On 2015/04/13 19:21:47, Devlin wrote: > RespondNow(Error()) sets the error. So you don't need line 488. Good catch, removed. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:499: manager->RemoveContentScripts(GetSenderWebContents(), view_instance_id, On 2015/04/13 19:21:47, Devlin wrote: > nit: > std::vector<std::string> script_name_list; > if (params->script_name_list) > script_name_list.swap(*params->script_name_list); > manager->RemoveContentScripts(GetSenderWebContents(), view_instance_id, > host_id, script_name_list); Thanks! https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... File extensions/browser/guest_view/guest_view_message_filter.h (right): https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/guest_view_message_filter.h:65: bool* allowed); On 2015/04/13 19:21:47, Devlin wrote: > nit: newline. Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.cc (right): https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:25: namespace { On 2015/04/13 19:21:48, Devlin wrote: > nit: This is long enough that there should be newlines around the namespace { } > and it should end in } // namespace. Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:27: HostID host_id, On 2015/04/13 19:21:48, Devlin wrote: > const & Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:36: if (!manager) On 2015/04/13 19:21:48, Devlin wrote: > We always pass in the same BrowserContext, and we do it from the manager - so > how could this fail? That said, we might actually be able to get rid of this. > See below. Use a DCHECK instead. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:50: class WebViewContentScriptManager::OwnerWebContentsObserver On 2015/04/13 19:21:48, Devlin wrote: > Document lifetime. Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:81: web_view_content_script_manager_->RemoveContentScripts(web_contents(), On 2015/04/13 19:21:47, Devlin wrote: > Document that this object can be deleted after this line. Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:141: key, std::map<std::string, UserScript>())); On 2015/04/13 19:21:48, Devlin wrote: > s/std::map<std::string, UserScript>()/ContentScriptMap() Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:152: master->RemoveScript(map_iter->second); On 2015/04/13 19:21:48, Devlin wrote: > We do an AttemptLoad() in UserScriptLoader after each call to > Remove/AddScript(). I think we should expose a batch method on the > UserScriptMaster to make this more efficient. Added. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:164: if (observers_map_iter == owner_web_contents_observer_map_.end()) { On 2015/04/13 19:21:48, Devlin wrote: > no reason to cache observers_map_iter, so just inline it in the if. Good catch, updated. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:167: owner_web_contents_observer_map_[embedder_web_contents] = observer; On 2015/04/13 19:21:48, Devlin wrote: > (See below to make sense of this) > observer->add_instance_id(view_instance_id); Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:175: base::Unretained(WebViewRendererState::GetInstance()), On 2015/04/13 19:21:48, Devlin wrote: > document why this unretained is safe. Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:216: names_to_delete = script_name_list; On 2015/04/13 19:21:48, Devlin wrote: > this is a potentially expensive copy. Use std::vector::swap instead. The disadvantage is that we have to pass in std::vector<std::string>, rather than const std::vector<std::string>& for script_name_list. I still prefer passing in a const &. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:244: // Step 1: removes content scripts of all the guests embedded. On 2015/04/13 19:21:48, Devlin wrote: > Why not instead store a std::set of view_instance_ids on the > OwnerWebContentsObservers, and then pass that in the RemoveContentScripts > method. That way, you can just loop over that, instead of having to do multiple > lookups. This is a great idea, the removal becomes much easier. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.h (right): https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:27: class WebViewContentScriptManager : public base::SupportsUserData::Data { On 2015/04/13 19:21:48, Devlin wrote: > Please file a bug and add a TODO here linking to it that we shouldn't have this > class managing webview lifetime (and possibly might not need it at all). Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:42: const std::map<std::string, UserScript>& user_scripts); On 2015/04/13 19:21:48, Devlin wrote: > The name is stored on the UserScript, so we don't need to pass in a map here. Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:86: content::BrowserContext* browser_context_; On 2015/04/13 19:21:48, Devlin wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_renderer_state.cc (right): https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_renderer_state.cc:122: for (int id : script_ids) { On 2015/04/13 19:21:48, Devlin wrote: > no brackets Done. https://codereview.chromium.org/959413003/diff/680001/extensions/common/api/w... File extensions/common/api/web_view_internal.json (right): https://codereview.chromium.org/959413003/diff/680001/extensions/common/api/w... extensions/common/api/web_view_internal.json:98: "id": "RunLocation", On 2015/04/13 19:21:48, Devlin wrote: > What was the error with trying to use ExtensionTypes' RunLocation? The namespace isn't generated properly in the generated .cc files. https://codereview.chromium.org/959413003/diff/680001/extensions/renderer/res... File extensions/renderer/resources/guest_view/web_view/web_view_api_methods.js (right): https://codereview.chromium.org/959413003/diff/680001/extensions/renderer/res... extensions/renderer/resources/guest_view/web_view/web_view_api_methods.js:99: var args = $Array.concat([this.viewInstanceId], $Array.slice(arguments)); On 2015/04/13 19:21:48, Devlin wrote: > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/a... > seems to think slicing on arguments is bad. I don't know much more than that, > though. Not an expert here, just follow the ways other functions do:( https://codereview.chromium.org/959413003/diff/680001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/959413003/diff/680001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:35: this->routing_id = routing_id; On 2015/04/13 19:21:48, Devlin wrote: > no need for "this" Updated.
https://codereview.chromium.org/959413003/diff/680001/chrome/test/data/extens... File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right): https://codereview.chromium.org/959413003/diff/680001/chrome/test/data/extens... chrome/test/data/extensions/platform_apps/web_view/shim/main.js:884: }, 1000); On 2015/04/14 19:05:50, Xi Han wrote: > On 2015/04/13 19:21:46, Devlin wrote: > > Why the timeout? > > Here we verify that we won't get any message from the first script which is > replaced by the second one, since they have the same name. 1000ms is a pretty arbitrary limit to put there. If everything works, it slows every run down by 1s (when you do thousands of runs per day, that can actually add up!), and if something doesn't work, it assumes that it will execute within that 1s - of which there is no guarantee. Can we not say that, since scripts execute in the order they were added, any message from a previous script will, at minimum, be in-flight by this point, and do timeout of 0? https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:95: true /* canExecuteScriptEverywhere */)); On 2015/04/14 19:05:51, Xi Han wrote: > On 2015/04/13 19:21:47, Devlin wrote: > > Why do we allow these to execute everywhere? > > I checked the implementation in ContentScriptsHandler for extensions, and it > looks up whether the extension is in the white list for scripts. Since we don't > specify any white list for <webview>, can we assume here should be true? No, quite the opposite. If we have no scripting white list for webview, it should *never* be true. That being said, I think this should actually be something along the lines of (pseudocode): bool allowed_everywhere = false; if (is_extension and ExtensionAllowedEverywhere or IsTrustedWebUI) allowed_everywhere = true because we, essentially, do have a whitelist for this. https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.cc (right): https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:216: names_to_delete = script_name_list; On 2015/04/14 19:05:51, Xi Han wrote: > On 2015/04/13 19:21:48, Devlin wrote: > > this is a potentially expensive copy. Use std::vector::swap instead. > > The disadvantage is that we have to pass in std::vector<std::string>, rather > than const std::vector<std::string>& for script_name_list. I still prefer > passing in a const &. To get around that: std::vector<std::string> generated_list; const std::vector<std::string>* names_to_delete = nullptr; if (script_name_list.empty()) { for (const std::pair<std::string, UserScript>& iter : map) generated_list.push_back(iter.first); names_to_delete = &generated_list; } else { names_to_delete = &script_name_list; } Though actually, since you're just going to then turn around and look up all the names, perhaps the better thing to do is: if (script_name_list.empty()) { for (const std::pair<std::string, UserScript>& iter : map) { scripts_to_delete.insert(iter->second); ids_to_delete.insert(iter->second.id()); } map.clear(); } else { for (const std::string& name : script_name_list) { ContentScriptMap::iterator iter = map.find(name); if (iter == map.end()) continue; scripts_to_delete.insert(iter->second); ids_to_delete.insert(iter->second.id()); map.erase(iter); } } Though it has a bit of duplicate code, it saves you some duplicate map lookups, and is actually not many (if any) more lines than what you have now. https://codereview.chromium.org/959413003/diff/680001/extensions/common/api/w... File extensions/common/api/web_view_internal.json (right): https://codereview.chromium.org/959413003/diff/680001/extensions/common/api/w... extensions/common/api/web_view_internal.json:98: "id": "RunLocation", On 2015/04/14 19:05:52, Xi Han wrote: > On 2015/04/13 19:21:48, Devlin wrote: > > What was the error with trying to use ExtensionTypes' RunLocation? > > The namespace isn't generated properly in the generated .cc files. That's a little vague - could you be more specific? https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:425: int view_instance_id = params->instance_id; why not just use params->instance_id? https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:431: std::map<std::string, UserScript> scripts; This map is still rather useless. Why not just generate it as a set? https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:445: script.set_id(UserScript::GenerateUserScriptID()); Why not do these in ParseContentScript(s)? https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:474: if (!view_instance_id) just use params->instance_id https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.h (right): https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.h:144: ExecuteCodeFunction::ResponseAction Run() final; While the "final" here is probably true, we usually don't use it unless there's a really compelling reason. https://codereview.chromium.org/959413003/diff/720001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.cc (right): https://codereview.chromium.org/959413003/diff/720001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:80: std::vector<int> view_instance_ids_; This should be a set. https://codereview.chromium.org/959413003/diff/720001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:143: // If a content script has the same name as the new one, removes the old s/removes/remove https://codereview.chromium.org/959413003/diff/720001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:162: observer->add_view_instance_id(view_instance_id); Your if guard on line 158 means each observer will only ever have one instance id. https://codereview.chromium.org/959413003/diff/720001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.h (right): https://codereview.chromium.org/959413003/diff/720001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:27: // TODO(hanxi): crbug.com/476938. Introduces a new class to manage the lifetime s/introduces/introduce https://codereview.chromium.org/959413003/diff/720001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:56: bool OwnsUserScript(int embedder_process_id, This doesn't exist? https://codereview.chromium.org/959413003/diff/720001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/959413003/diff/720001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:879: if (manager) { When can this be null?
Hi Devlin, PTAL, thanks! https://codereview.chromium.org/959413003/diff/680001/chrome/test/data/extens... File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right): https://codereview.chromium.org/959413003/diff/680001/chrome/test/data/extens... chrome/test/data/extensions/platform_apps/web_view/shim/main.js:884: }, 1000); On 2015/04/14 23:04:57, Devlin wrote: > On 2015/04/14 19:05:50, Xi Han wrote: > > On 2015/04/13 19:21:46, Devlin wrote: > > > Why the timeout? > > > > Here we verify that we won't get any message from the first script which is > > replaced by the second one, since they have the same name. > > 1000ms is a pretty arbitrary limit to put there. If everything works, it slows > every run down by 1s (when you do thousands of runs per day, that can actually > add up!), and if something doesn't work, it assumes that it will execute within > that 1s - of which there is no guarantee. Can we not say that, since scripts > execute in the order they were added, any message from a previous script will, > at minimum, be in-flight by this point, and do timeout of 0? Sounds good to me, updated:) https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:95: true /* canExecuteScriptEverywhere */)); On 2015/04/14 23:04:57, Devlin wrote: > On 2015/04/14 19:05:51, Xi Han wrote: > > On 2015/04/13 19:21:47, Devlin wrote: > > > Why do we allow these to execute everywhere? > > > > I checked the implementation in ContentScriptsHandler for extensions, and it > > looks up whether the extension is in the white list for scripts. Since we > don't > > specify any white list for <webview>, can we assume here should be true? > > No, quite the opposite. If we have no scripting white list for webview, it > should *never* be true. That being said, I think this should actually be > something along the lines of (pseudocode): > bool allowed_everywhere = false; > if (is_extension and ExtensionAllowedEverywhere or IsTrustedWebUI) > allowed_everywhere = true > > because we, essentially, do have a whitelist for this. I see. Can we assume that we can trust webUI? https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.cc (right): https://codereview.chromium.org/959413003/diff/680001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:216: names_to_delete = script_name_list; On 2015/04/14 23:04:57, Devlin wrote: > On 2015/04/14 19:05:51, Xi Han wrote: > > On 2015/04/13 19:21:48, Devlin wrote: > > > this is a potentially expensive copy. Use std::vector::swap instead. > > > > The disadvantage is that we have to pass in std::vector<std::string>, rather > > than const std::vector<std::string>& for script_name_list. I still prefer > > passing in a const &. > > To get around that: > std::vector<std::string> generated_list; > const std::vector<std::string>* names_to_delete = nullptr; > if (script_name_list.empty()) { > for (const std::pair<std::string, UserScript>& iter : map) > generated_list.push_back(iter.first); > names_to_delete = &generated_list; > } else { > names_to_delete = &script_name_list; > } > > Though actually, since you're just going to then turn around and look up all the > names, perhaps the better thing to do is: > > if (script_name_list.empty()) { > for (const std::pair<std::string, UserScript>& iter : map) { > scripts_to_delete.insert(iter->second); > ids_to_delete.insert(iter->second.id()); > } > map.clear(); > } else { > for (const std::string& name : script_name_list) { > ContentScriptMap::iterator iter = map.find(name); > if (iter == map.end()) > continue; > scripts_to_delete.insert(iter->second); > ids_to_delete.insert(iter->second.id()); > map.erase(iter); > } > } > > Though it has a bit of duplicate code, it saves you some duplicate map lookups, > and is actually not many (if any) more lines than what you have now. The second suggestion was just my previous implementation. Revert back. https://codereview.chromium.org/959413003/diff/680001/extensions/common/api/w... File extensions/common/api/web_view_internal.json (right): https://codereview.chromium.org/959413003/diff/680001/extensions/common/api/w... extensions/common/api/web_view_internal.json:98: "id": "RunLocation", On 2015/04/14 23:04:57, Devlin wrote: > On 2015/04/14 19:05:52, Xi Han wrote: > > On 2015/04/13 19:21:48, Devlin wrote: > > > What was the error with trying to use ExtensionTypes' RunLocation? > > > > The namespace isn't generated properly in the generated .cc files. > > That's a little vague - could you be more specific? Sorry, I forget to come back to paste the compile error here: gen/extensions/common/api/web_view_internal.cc:522:23: error: use of undeclared identifier 'RUN_AT_NONE'; did you mean 'extension_types::RUN_AT_NONE'? if (this->run_at != RUN_AT_NONE) { ^~~~~~~~~~~ extension_types::RUN_AT_NONE gen/extensions/common/api/extension_types.h:74:3: note: 'extension_types::RUN_AT_NONE' declared here RUN_AT_NONE, ^ I checked all the $ref from extension_types.h in the same json file, and all of them are defined as a parameter of a function. While my case is an id in properties. The compiler can not properly generate the namespace for it. https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:425: int view_instance_id = params->instance_id; On 2015/04/14 23:04:57, Devlin wrote: > why not just use params->instance_id? Since later we will use view_instance_id again (line453), it's better to cache it. https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:431: std::map<std::string, UserScript> scripts; On 2015/04/14 23:04:57, Devlin wrote: > This map is still rather useless. Why not just generate it as a set? This map is used to deal with the name duplications. If the user adds a few scripts with the same names, we can only add the last script. https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:445: script.set_id(UserScript::GenerateUserScriptID()); On 2015/04/14 23:04:57, Devlin wrote: > Why not do these in ParseContentScript(s)? It could, but I hope the ParseContentScript(s) only deals with parsing the data. It would be great if other initialization could be in one place. But if you like to set_id when parsing, I am ok with it too. https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:474: if (!view_instance_id) On 2015/04/14 23:04:57, Devlin wrote: > just use params->instance_id Same as above. It is used in line 487. https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.h (right): https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.h:144: ExecuteCodeFunction::ResponseAction Run() final; On 2015/04/14 23:04:57, Devlin wrote: > While the "final" here is probably true, we usually don't use it unless there's > a really compelling reason. I think I got compiling errors when I first used override, so I changed to finial. But now it doesn't have any error anymore. Reverted back. https://codereview.chromium.org/959413003/diff/720001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.cc (right): https://codereview.chromium.org/959413003/diff/720001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:80: std::vector<int> view_instance_ids_; On 2015/04/14 23:04:57, Devlin wrote: > This should be a set. Done. https://codereview.chromium.org/959413003/diff/720001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:143: // If a content script has the same name as the new one, removes the old On 2015/04/14 23:04:57, Devlin wrote: > s/removes/remove Done. https://codereview.chromium.org/959413003/diff/720001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:162: observer->add_view_instance_id(view_instance_id); On 2015/04/14 23:04:57, Devlin wrote: > Your if guard on line 158 means each observer will only ever have one instance > id. Great catch, it is a bug. Fixed. https://codereview.chromium.org/959413003/diff/720001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.h (right): https://codereview.chromium.org/959413003/diff/720001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:27: // TODO(hanxi): crbug.com/476938. Introduces a new class to manage the lifetime On 2015/04/14 23:04:57, Devlin wrote: > s/introduces/introduce Done. https://codereview.chromium.org/959413003/diff/720001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.h:56: bool OwnsUserScript(int embedder_process_id, On 2015/04/14 23:04:57, Devlin wrote: > This doesn't exist? Oh, it is legacy code and should be removed. https://codereview.chromium.org/959413003/diff/720001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/959413003/diff/720001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:879: if (manager) { On 2015/04/14 23:04:57, Devlin wrote: > When can this be null? Change to DCHECK.
https://codereview.chromium.org/959413003/diff/740001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/740001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:96: bool allowd_everywhere = false; nit: allowed_everywhere
https://codereview.chromium.org/959413003/diff/740001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/740001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:96: bool allowd_everywhere = false; On 2015/04/15 19:48:39, Fady Samuel wrote: > nit: allowed_everywhere Sorry for the typo. Updated.
xiyuan@chromium.org changed reviewers: + xiyuan@chromium.org
https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:95: true /* canExecuteScriptEverywhere */)); On 2015/04/15 19:43:55, Xi Han wrote: > On 2015/04/14 23:04:57, Devlin wrote: > > On 2015/04/14 19:05:51, Xi Han wrote: > > > On 2015/04/13 19:21:47, Devlin wrote: > > > > Why do we allow these to execute everywhere? > > > > > > I checked the implementation in ContentScriptsHandler for extensions, and it > > > looks up whether the extension is in the white list for scripts. Since we > > don't > > > specify any white list for <webview>, can we assume here should be true? > > > > No, quite the opposite. If we have no scripting white list for webview, it > > should *never* be true. That being said, I think this should actually be > > something along the lines of (pseudocode): > > bool allowed_everywhere = false; > > if (is_extension and ExtensionAllowedEverywhere or IsTrustedWebUI) > > allowed_everywhere = true > > > > because we, essentially, do have a whitelist for this. > > I see. Can we assume that we can trust webUI? Not sure about the webivew in app case. But for webview in webui, we probably can use |false| since it should be sufficient for sign-in webui to inject only into http/https.
https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:95: true /* canExecuteScriptEverywhere */)); On 2015/04/15 19:56:10, xiyuan wrote: > On 2015/04/15 19:43:55, Xi Han wrote: > > On 2015/04/14 23:04:57, Devlin wrote: > > > On 2015/04/14 19:05:51, Xi Han wrote: > > > > On 2015/04/13 19:21:47, Devlin wrote: > > > > > Why do we allow these to execute everywhere? > > > > > > > > I checked the implementation in ContentScriptsHandler for extensions, and > it > > > > looks up whether the extension is in the white list for scripts. Since we > > > don't > > > > specify any white list for <webview>, can we assume here should be true? > > > > > > No, quite the opposite. If we have no scripting white list for webview, it > > > should *never* be true. That being said, I think this should actually be > > > something along the lines of (pseudocode): > > > bool allowed_everywhere = false; > > > if (is_extension and ExtensionAllowedEverywhere or IsTrustedWebUI) > > > allowed_everywhere = true > > > > > > because we, essentially, do have a whitelist for this. > > > > I see. Can we assume that we can trust webUI? > > Not sure about the webivew in app case. But for webview in webui, we probably > can use |false| since it should be sufficient for sign-in webui to inject only > into http/https. If that is the case, we can just use |false| for webview of webUI now:)
On 2015/04/15 20:13:49, Xi Han wrote: > https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... > File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc > (right): > > https://codereview.chromium.org/959413003/diff/680001/extensions/browser/api/... > extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:95: true /* > canExecuteScriptEverywhere */)); > On 2015/04/15 19:56:10, xiyuan wrote: > > On 2015/04/15 19:43:55, Xi Han wrote: > > > On 2015/04/14 23:04:57, Devlin wrote: > > > > On 2015/04/14 19:05:51, Xi Han wrote: > > > > > On 2015/04/13 19:21:47, Devlin wrote: > > > > > > Why do we allow these to execute everywhere? > > > > > > > > > > I checked the implementation in ContentScriptsHandler for extensions, > and > > it > > > > > looks up whether the extension is in the white list for scripts. Since > we > > > > don't > > > > > specify any white list for <webview>, can we assume here should be true? > > > > > > > > > No, quite the opposite. If we have no scripting white list for webview, > it > > > > should *never* be true. That being said, I think this should actually be > > > > something along the lines of (pseudocode): > > > > bool allowed_everywhere = false; > > > > if (is_extension and ExtensionAllowedEverywhere or IsTrustedWebUI) > > > > allowed_everywhere = true > > > > > > > > because we, essentially, do have a whitelist for this. > > > > > > I see. Can we assume that we can trust webUI? > > > > Not sure about the webivew in app case. But for webview in webui, we probably > > can use |false| since it should be sufficient for sign-in webui to inject only > > into http/https. > > If that is the case, we can just use |false| for webview of webUI now:) Yep, that seems to be working fine.
https://codereview.chromium.org/959413003/diff/680001/extensions/common/api/w... File extensions/common/api/web_view_internal.json (right): https://codereview.chromium.org/959413003/diff/680001/extensions/common/api/w... extensions/common/api/web_view_internal.json:98: "id": "RunLocation", On 2015/04/15 19:43:55, Xi Han wrote: > On 2015/04/14 23:04:57, Devlin wrote: > > On 2015/04/14 19:05:52, Xi Han wrote: > > > On 2015/04/13 19:21:48, Devlin wrote: > > > > What was the error with trying to use ExtensionTypes' RunLocation? > > > > > > The namespace isn't generated properly in the generated .cc files. > > > > That's a little vague - could you be more specific? > > Sorry, I forget to come back to paste the compile error here: > gen/extensions/common/api/web_view_internal.cc:522:23: error: use of undeclared > identifier 'RUN_AT_NONE'; did you mean 'extension_types::RUN_AT_NONE'? > if (this->run_at != RUN_AT_NONE) { > ^~~~~~~~~~~ > extension_types::RUN_AT_NONE > gen/extensions/common/api/extension_types.h:74:3: note: > 'extension_types::RUN_AT_NONE' declared here > RUN_AT_NONE, > ^ > I checked all the $ref from extension_types.h in the same json file, and all of > them are defined as a parameter of a function. While my case is an id in > properties. The compiler can not properly generate the namespace for it. Oh, lame. Can you file a bug for that, and assign it to me (and cc kalman@)? https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:425: int view_instance_id = params->instance_id; On 2015/04/15 19:43:55, Xi Han wrote: > On 2015/04/14 23:04:57, Devlin wrote: > > why not just use params->instance_id? > > Since later we will use view_instance_id again (line453), it's better to cache > it. The compiler will definitely optimize this out (the same can't always be said when getting an object from a function, since the compiler doesn't know if that function has side effects), and it makes it confusing to have two variables that mean the same thing. https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:431: std::map<std::string, UserScript> scripts; On 2015/04/15 19:43:55, Xi Han wrote: > On 2015/04/14 23:04:57, Devlin wrote: > > This map is still rather useless. Why not just generate it as a set? > > This map is used to deal with the name duplications. If the user adds a few > scripts with the same names, we can only add the last script. I think if the user is trying to add two scripts with the same name in the same function call, we should treat it as an error, and abort - it's almost certainly an accident. (Adding a content script with the same name later is okay to overwrite.) https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:445: script.set_id(UserScript::GenerateUserScriptID()); On 2015/04/15 19:43:55, Xi Han wrote: > On 2015/04/14 23:04:57, Devlin wrote: > > Why not do these in ParseContentScript(s)? > > It could, but I hope the ParseContentScript(s) only deals with parsing the data. > It would be great if other initialization could be in one place. But if you like > to set_id when parsing, I am ok with it too. I think I'd like to have the script generation all done in one place - it makes it easier to see if we're missing something. Also, that, coupled with not using the map, saves us two copies when generating the set (one on line 444, and another potential on line 450). https://codereview.chromium.org/959413003/diff/780001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/780001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:95: bool allowed_everywhere = false; Might be worth making a note here that we default WebUI to not having special access, but we can change that if needed.
Patchset #22 (id:800001) has been deleted
PTAL, thanks:) https://codereview.chromium.org/959413003/diff/680001/extensions/common/api/w... File extensions/common/api/web_view_internal.json (right): https://codereview.chromium.org/959413003/diff/680001/extensions/common/api/w... extensions/common/api/web_view_internal.json:98: "id": "RunLocation", On 2015/04/15 20:58:05, Devlin wrote: > On 2015/04/15 19:43:55, Xi Han wrote: > > On 2015/04/14 23:04:57, Devlin wrote: > > > On 2015/04/14 19:05:52, Xi Han wrote: > > > > On 2015/04/13 19:21:48, Devlin wrote: > > > > > What was the error with trying to use ExtensionTypes' RunLocation? > > > > > > > > The namespace isn't generated properly in the generated .cc files. > > > > > > That's a little vague - could you be more specific? > > > > Sorry, I forget to come back to paste the compile error here: > > gen/extensions/common/api/web_view_internal.cc:522:23: error: use of > undeclared > > identifier 'RUN_AT_NONE'; did you mean 'extension_types::RUN_AT_NONE'? > > if (this->run_at != RUN_AT_NONE) { > > ^~~~~~~~~~~ > > extension_types::RUN_AT_NONE > > gen/extensions/common/api/extension_types.h:74:3: note: > > 'extension_types::RUN_AT_NONE' declared here > > RUN_AT_NONE, > > ^ > > I checked all the $ref from extension_types.h in the same json file, and all > of > > them are defined as a parameter of a function. While my case is an id in > > properties. The compiler can not properly generate the namespace for it. > > Oh, lame. Can you file a bug for that, and assign it to me (and cc kalman@)? sure, thanks for looking into this issue. https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:425: int view_instance_id = params->instance_id; On 2015/04/15 20:58:05, Devlin wrote: > On 2015/04/15 19:43:55, Xi Han wrote: > > On 2015/04/14 23:04:57, Devlin wrote: > > > why not just use params->instance_id? > > > > Since later we will use view_instance_id again (line453), it's better to cache > > it. > > The compiler will definitely optimize this out (the same can't always be said > when getting an object from a function, since the compiler doesn't know if that > function has side effects), and it makes it confusing to have two variables that > mean the same thing. That is fine, updated. https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:431: std::map<std::string, UserScript> scripts; On 2015/04/15 20:58:05, Devlin wrote: > On 2015/04/15 19:43:55, Xi Han wrote: > > On 2015/04/14 23:04:57, Devlin wrote: > > > This map is still rather useless. Why not just generate it as a set? > > > > This map is used to deal with the name duplications. If the user adds a few > > scripts with the same names, we can only add the last script. > > I think if the user is trying to add two scripts with the same name in the same > function call, we should treat it as an error, and abort - it's almost certainly > an accident. (Adding a content script with the same name later is okay to > overwrite.) That is ok, I use a set to check the duplication of names when API is called, and return false if seeing duplicated names. https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:445: script.set_id(UserScript::GenerateUserScriptID()); On 2015/04/15 20:58:05, Devlin wrote: > On 2015/04/15 19:43:55, Xi Han wrote: > > On 2015/04/14 23:04:57, Devlin wrote: > > > Why not do these in ParseContentScript(s)? > > > > It could, but I hope the ParseContentScript(s) only deals with parsing the > data. > > It would be great if other initialization could be in one place. But if you > like > > to set_id when parsing, I am ok with it too. > > I think I'd like to have the script generation all done in one place - it makes > it easier to see if we're missing something. > > Also, that, coupled with not using the map, saves us two copies when generating > the set (one on line 444, and another potential on line 450). Yes, as you suggested, I also move the initilization to the ParseContentScripts(). https://codereview.chromium.org/959413003/diff/720001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:474: if (!view_instance_id) On 2015/04/15 19:43:55, Xi Han wrote: > On 2015/04/14 23:04:57, Devlin wrote: > > just use params->instance_id > > Same as above. It is used in line 487. Done. https://codereview.chromium.org/959413003/diff/780001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/780001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:95: bool allowed_everywhere = false; On 2015/04/15 20:58:05, Devlin wrote: > Might be worth making a note here that we default WebUI to not having special > access, but we can change that if needed. Added.
https://codereview.chromium.org/959413003/diff/820001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/820001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:95: // The defaul for WebUI is not having special access, but we can change that nit: default
https://codereview.chromium.org/959413003/diff/820001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.cc (right): https://codereview.chromium.org/959413003/diff/820001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:51: // If the embedder navigates to a different page then destroy the guest. This comment is wrong. If the embedder navigates to a different page then remove all the content scripts of the guest. https://codereview.chromium.org/959413003/diff/820001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:56: // If the embedder crashes, then destroy the guest. If the embedder crashes, then remove all the content scripts of the guest.
https://codereview.chromium.org/959413003/diff/820001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/820001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:95: // The defaul for WebUI is not having special access, but we can change that On 2015/04/16 15:24:55, Fady Samuel wrote: > nit: default Done. https://codereview.chromium.org/959413003/diff/820001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_content_script_manager.cc (right): https://codereview.chromium.org/959413003/diff/820001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:51: // If the embedder navigates to a different page then destroy the guest. On 2015/04/16 15:29:05, Fady Samuel wrote: > This comment is wrong. > > If the embedder navigates to a different page then remove all the content > scripts of the guest. Good catch, thanks. https://codereview.chromium.org/959413003/diff/820001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_content_script_manager.cc:56: // If the embedder crashes, then destroy the guest. On 2015/04/16 15:29:05, Fady Samuel wrote: > If the embedder crashes, then remove all the content scripts of the guest. Done.
LGTM https://codereview.chromium.org/959413003/diff/840001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/840001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:91: const std::vector<std::string>& matches = script_value.matches; nit: inline matches https://codereview.chromium.org/959413003/diff/840001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:92: if (matches.size() == 0) nit: prefer if (matches.empty()) https://codereview.chromium.org/959413003/diff/840001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:141: script->set_run_location(run_at); might be worth commenting that this defaults to document idle. https://codereview.chromium.org/959413003/diff/840001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:212: if (names.find(name) != names.end()) This can be simplified to if (!names.insert(name).second) { // The name was already in the list. } https://codereview.chromium.org/959413003/diff/840001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:213: return false; set an error https://codereview.chromium.org/959413003/diff/840001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/959413003/diff/840001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:58: // Even if the script is removed by the webview, the user scipt will be removed The third section of this comment ('Even if...') sounds weird to me. Can you rephrase?
hanxi@chromium.org changed reviewers: + kenrb@chromium.org
kenrb@chromium.org: Please review changes in -extensions/common/guest_view/guest_view_messages.h Thanks! https://codereview.chromium.org/959413003/diff/840001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/959413003/diff/840001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:91: const std::vector<std::string>& matches = script_value.matches; On 2015/04/16 16:17:39, Devlin wrote: > nit: inline matches Done. https://codereview.chromium.org/959413003/diff/840001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:92: if (matches.size() == 0) On 2015/04/16 16:17:39, Devlin wrote: > nit: prefer if (matches.empty()) Done. https://codereview.chromium.org/959413003/diff/840001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:141: script->set_run_location(run_at); On 2015/04/16 16:17:39, Devlin wrote: > might be worth commenting that this defaults to document idle. Done. https://codereview.chromium.org/959413003/diff/840001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:212: if (names.find(name) != names.end()) On 2015/04/16 16:17:39, Devlin wrote: > This can be simplified to > if (!names.insert(name).second) { > // The name was already in the list. > } Done. https://codereview.chromium.org/959413003/diff/840001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:213: return false; On 2015/04/16 16:17:39, Devlin wrote: > set an error Done. https://codereview.chromium.org/959413003/diff/840001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/959413003/diff/840001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:58: // Even if the script is removed by the webview, the user scipt will be removed On 2015/04/16 16:17:39, Devlin wrote: > The third section of this comment ('Even if...') sounds weird to me. Can you > rephrase? Done.
ipc lgtm
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/959413003/#ps860001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/959413003/860001
Message was sent while issue was closed.
Committed patchset #24 (id:860001)
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/05a02505462f29d067288ec9e5143c2daad5be78 Cr-Commit-Position: refs/heads/master@{#325492}
Message was sent while issue was closed.
https://codereview.chromium.org/959413003/diff/860001/extensions/browser/exte... File extensions/browser/extension_function_histogram_value.h (right): https://codereview.chromium.org/959413003/diff/860001/extensions/browser/exte... extensions/browser/extension_function_histogram_value.h:1068: // tools/metrics/histograms/histograms.xml. For future reference, this ^^. You need to update histograms.xml. You should have also gotten a warning for not having a valid reviewer for extension_function_histogram_value.h (you're supposed to need a metrics reviewer). I've updated histograms.xml to include these in crrev.com/b2eb97880d384168ae17cfe24b3c874d72428141 so no need to for this patch.
Message was sent while issue was closed.
On 2015/04/17 00:29:23, Devlin wrote: > https://codereview.chromium.org/959413003/diff/860001/extensions/browser/exte... > File extensions/browser/extension_function_histogram_value.h (right): > > https://codereview.chromium.org/959413003/diff/860001/extensions/browser/exte... > extensions/browser/extension_function_histogram_value.h:1068: // > tools/metrics/histograms/histograms.xml. > For future reference, this ^^. You need to update histograms.xml. > > You should have also gotten a warning for not having a valid reviewer for > extension_function_histogram_value.h (you're supposed to need a metrics > reviewer). > > I've updated histograms.xml to include these in > crrev.com/b2eb97880d384168ae17cfe24b3c874d72428141 so no need to for this patch. Thank you Devlin, I would update histograms.xml if I knew that I need to change histograms.xml. I didn't get any warning since I TBR the owner of extension_function_histogram_value.h. Thanks for letting me know again:)
Message was sent while issue was closed.
On 2015/04/17 12:50:32, Xi Han wrote: > Thank you Devlin, I would update histograms.xml if I knew that I need to change > histograms.xml. I didn't get any warning since I TBR the owner of > extension_function_histogram_value.h. Thanks for letting me know again:) Ah, missed the TBR. That explains it. In the future, please use TBR judiciously. It's meant for minor/mechanical changes. Also, the reason extension_function_histogram_values.h has a different set of OWNERS than extensions/ is because almost any change to the metrics file is significant enough to warrant a real review by a metrics OWNER. Also, usually TBR etiquette is to still add the TBR'ed reviewer to the list of reviewers, so that they can still see the patch (TBR means To Be Reviewed, so it should still be seen by them at some point).
Message was sent while issue was closed.
hanxi@chromium.org changed reviewers: + asvitkine@chromium.org
Message was sent while issue was closed.
On 2015/04/17 15:48:11, Devlin wrote: > On 2015/04/17 12:50:32, Xi Han wrote: > > Thank you Devlin, I would update histograms.xml if I knew that I need to > change > > histograms.xml. I didn't get any warning since I TBR the owner of > > extension_function_histogram_value.h. Thanks for letting me know again:) > > Ah, missed the TBR. That explains it. > > In the future, please use TBR judiciously. It's meant for minor/mechanical > changes. Also, the reason extension_function_histogram_values.h has a different > set of OWNERS than extensions/ is because almost any change to the metrics file > is significant enough to warrant a real review by a metrics OWNER. > > Also, usually TBR etiquette is to still add the TBR'ed reviewer to the list of > reviewers, so that they can still see the patch (TBR means To Be Reviewed, so it > should still be seen by them at some point). Got it, added the asvitkine@ as a reviewer, and will send my future patch to a real reviewer if I make changes in extension_function_histogram_values.h. |
