Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
906493004:
Refactoring: de-couple Extensions from "script injection System" [render side]:2 (Closed)
|
Created:
5 years, 10 months ago by Xi Han Modified:
5 years, 9 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@decouple_brower_isolated_world_1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis CL adds routing info for content scripts from <webview>.
To support dynamically added/removed content scripts in <webview>, we store
these content scripts in the same shared memory where user scripts from
declarative content API are stored, but the decision for injection will
be made in the render once the url pattern is matched. Since render doesn't
need to send IPCs to ask a decision from browser, it makes a precise time
injection possible.
Special to <webview>, once the content scripts are updated (added/removed),
browser will only notify the render process of the given <webview>, rather than
all of the renders (which is the case for user scripts).
BUG=437566
Committed: https://crrev.com/f853287c95cf818d0b5b02d7f4a6588761096d1c
Cr-Commit-Position: refs/heads/master@{#318774}
Committed: https://crrev.com/ccff496a48cdf435d59ef79f3fe9ae1e6914ed22
Cr-Commit-Position: refs/heads/master@{#318959}
Patch Set 1 : #Patch Set 2 : Rebase. #
Total comments: 8
Patch Set 3 : Devlin's comments. #Patch Set 4 : Fady's comments #Patch Set 5 : Rebase and remove ConsumerInstanceInfo(id). #
Total comments: 17
Patch Set 6 : Devlin's comments and rebase. #
Total comments: 4
Patch Set 7 : ScriptInjection::CanExecuteOnFrame #Patch Set 8 : clean up. #
Total comments: 3
Patch Set 9 : resubmit #
Total comments: 6
Messages
Total messages: 47 (17 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
hanxi@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Hi Devlin: Please review all changes, thanks a lot!
Patchset #1 (id:40001) has been deleted
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
https://codereview.chromium.org/906493004/diff/80001/chrome/browser/extension... File chrome/browser/extensions/user_script_loader.cc (right): https://codereview.chromium.org/906493004/diff/80001/chrome/browser/extension... chrome/browser/extensions/user_script_loader.cc:463: if (i.GetCurrentValue()->GetID() != render_process_id) { if (is_web_view && i.GetCurrentValue()->GetID() != render_process_id) continue; ... https://codereview.chromium.org/906493004/diff/80001/chrome/browser/extension... chrome/browser/extensions/user_script_loader.cc:465: } else { Not necessary.
I'm hesitant to review this too much before the dependent patch is done, but a few quick comments. https://codereview.chromium.org/906493004/diff/80001/extensions/common/user_s... File extensions/common/user_script.h (right): https://codereview.chromium.org/906493004/diff/80001/extensions/common/user_s... extensions/common/user_script.h:217: void set_consumer_instance_info(const ConsumerInstanceInfo& instance_info) { I don't see these being set anywhere yet - are they supposed to be? https://codereview.chromium.org/906493004/diff/80001/extensions/renderer/scri... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/906493004/diff/80001/extensions/renderer/scri... extensions/renderer/script_injection.cc:187: void ScriptInjection::RequestPermission() { This should be a no-op if this is from a webui.
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Hi Devlin: I would like you to review both CLs together, since if you find anything not reasonable, I can make changes immediately:) These two CLs don't have many dependencies (hopefully), and I try to do something special for <webview> in this CL. Thanks again for reviewing my patches:) https://codereview.chromium.org/906493004/diff/80001/chrome/browser/extension... File chrome/browser/extensions/user_script_loader.cc (right): https://codereview.chromium.org/906493004/diff/80001/chrome/browser/extension... chrome/browser/extensions/user_script_loader.cc:465: } else { On 2015/02/11 19:08:22, Fady Samuel wrote: > Not necessary. I add the else for early break (line 467 "break"). This is because each webview will only have one renderProcessHost, once we find that one, we will send message and break from the loop. https://codereview.chromium.org/906493004/diff/80001/extensions/common/user_s... File extensions/common/user_script.h (right): https://codereview.chromium.org/906493004/diff/80001/extensions/common/user_s... extensions/common/user_script.h:217: void set_consumer_instance_info(const ConsumerInstanceInfo& instance_info) { On 2015/02/11 20:29:01, Devlin wrote: > I don't see these being set anywhere yet - are they supposed to be? Yes, once we have add/remove content scripts APIs for <webviews>, the webview will set the consumer_instance_info when initializing the user scripts. https://codereview.chromium.org/906493004/diff/80001/extensions/renderer/scri... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/906493004/diff/80001/extensions/renderer/scri... extensions/renderer/script_injection.cc:187: void ScriptInjection::RequestPermission() { On 2015/02/11 20:29:01, Devlin wrote: > This should be a no-op if this is from a webui. Oh, I see. Even ExtensionHostMsg_RequestScriptInjectionPermission with request = -1 will updated permitted_extensions list, which is for extensions only. Updated.
https://codereview.chromium.org/906493004/diff/80001/chrome/browser/extension... File chrome/browser/extensions/user_script_loader.cc (right): https://codereview.chromium.org/906493004/diff/80001/chrome/browser/extension... chrome/browser/extensions/user_script_loader.cc:465: } else { Adopt Fady's suggestion.
Patchset #5 (id:180001) has been deleted
https://codereview.chromium.org/906493004/diff/200001/extensions/common/user_... File extensions/common/user_script.h (right): https://codereview.chromium.org/906493004/diff/200001/extensions/common/user_... extensions/common/user_script.h:312: // The type of the consumer instance that the script will be injected. It seems like we're adding a bunch of data to the user script, but I'd hope it's not all necessary. Unfortunately, it's a bit hard to tell from this patch. Would it be possible to make this patch a bit larger, or to upload subsequent patches, so we can see some of the bigger picture?
As discussed offline, we will keep these routing info in the UserScript for now. Please take another look of this CL. Thanks for reviewing it! On Tue, Feb 17, 2015 at 1:58 PM, <rdevlin.cronin@chromium.org> wrote: > > https://codereview.chromium.org/906493004/diff/200001/ > extensions/common/user_script.h > File extensions/common/user_script.h (right): > > https://codereview.chromium.org/906493004/diff/200001/ > extensions/common/user_script.h#newcode312 > extensions/common/user_script.h:312: // The type of the consumer > instance that the script will be injected. > It seems like we're adding a bunch of data to the user script, but I'd > hope it's not all necessary. Unfortunately, it's a bit hard to tell > from this patch. Would it be possible to make this patch a bit larger, > or to upload subsequent patches, so we can see some of the bigger > picture? > > https://codereview.chromium.org/906493004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/906493004/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.cc (right): https://codereview.chromium.org/906493004/diff/200001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:449: const UserScriptList* scripts = user_scripts.get(); Why cache this, isntead of just using user_scripts? https://codereview.chromium.org/906493004/diff/200001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:453: DCHECK(scripts->size() == 1); nit: DCHECK_EQ(1u, scripts->size()); https://codereview.chromium.org/906493004/diff/200001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:458: for (content::RenderProcessHost::iterator i( I think this is clearer as: if (is_web_view) { RenderProcessHost* host = RenderProcessHost::FromId(id); if (host) SendUpdate(host...); } else { for (content::RPH::iterator i(content::RPH::AllHostsIterator(); !i.IsAtEnd(); i.Advance()) { SendUpdate(i.GetCurrentValue()...); } } This also means that you can actually replace |is_web_view| the content of the if statement on line 450-452. https://codereview.chromium.org/906493004/diff/200001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/906493004/diff/200001/extensions/renderer/scr... extensions/renderer/script_injection.cc:179: if (host_id_.type() != HostID::EXTENSIONS) I think this needs to be rebased. https://codereview.chromium.org/906493004/diff/200001/extensions/renderer/scr... File extensions/renderer/script_injection.h (right): https://codereview.chromium.org/906493004/diff/200001/extensions/renderer/scr... extensions/renderer/script_injection.h:37: const UserScript::ConsumerInstanceType& consumer_instance_type, nit: a const& enum is kinda silly. https://codereview.chromium.org/906493004/diff/200001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/906493004/diff/200001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:138: if (script_->routing_info().render_view_id == routing_id && Should this just work with the injection host call?
https://codereview.chromium.org/906493004/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.cc (right): https://codereview.chromium.org/906493004/diff/200001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:458: for (content::RenderProcessHost::iterator i( On 2015/02/23 20:15:00, Devlin wrote: > I think this is clearer as: > if (is_web_view) { > RenderProcessHost* host = RenderProcessHost::FromId(id); > if (host) > SendUpdate(host...); > } else { > for (content::RPH::iterator i(content::RPH::AllHostsIterator(); > !i.IsAtEnd(); i.Advance()) { > SendUpdate(i.GetCurrentValue()...); > } > } > > This also means that you can actually replace |is_web_view| the content of the > if statement on line 450-452. Does this mean that there is one UserScriptLoader per <webview>?
Patchset #6 (id:220001) has been deleted
Patchset #6 (id:240001) has been deleted
Patchset #6 (id:250001) has been deleted
https://codereview.chromium.org/906493004/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.cc (right): https://codereview.chromium.org/906493004/diff/200001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:449: const UserScriptList* scripts = user_scripts.get(); On 2015/02/23 20:15:00, Devlin wrote: > Why cache this, isntead of just using user_scripts? Each of them is fine to me. Update. https://codereview.chromium.org/906493004/diff/200001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:453: DCHECK(scripts->size() == 1); On 2015/02/23 20:15:00, Devlin wrote: > nit: DCHECK_EQ(1u, scripts->size()); Done. https://codereview.chromium.org/906493004/diff/200001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:458: for (content::RenderProcessHost::iterator i( On 2015/02/23 20:15:00, Devlin wrote: > I think this is clearer as: > if (is_web_view) { > RenderProcessHost* host = RenderProcessHost::FromId(id); > if (host) > SendUpdate(host...); > } else { > for (content::RPH::iterator i(content::RPH::AllHostsIterator(); > !i.IsAtEnd(); i.Advance()) { > SendUpdate(i.GetCurrentValue()...); > } > } > > This also means that you can actually replace |is_web_view| the content of the > if statement on line 450-452. Great idea, the logic is much clear now. https://codereview.chromium.org/906493004/diff/200001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:458: for (content::RenderProcessHost::iterator i( > Does this mean that there is one UserScriptLoader per <webview>? We still have UserScriptLoader per hostID, user scripts from the same hostID (may have different webviews and tabs) will be saved in the same shared memory region. The changes here is: if we find out a user script is from webview, we only send notification from the specified RenderViewHost of that webview, rather than all the RenderViewHost. https://codereview.chromium.org/906493004/diff/200001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/906493004/diff/200001/extensions/renderer/scr... extensions/renderer/script_injection.cc:179: if (host_id_.type() != HostID::EXTENSIONS) On 2015/02/23 20:15:00, Devlin wrote: > I think this needs to be rebased. Done. https://codereview.chromium.org/906493004/diff/200001/extensions/renderer/scr... File extensions/renderer/script_injection.h (right): https://codereview.chromium.org/906493004/diff/200001/extensions/renderer/scr... extensions/renderer/script_injection.h:37: const UserScript::ConsumerInstanceType& consumer_instance_type, On 2015/02/23 20:15:00, Devlin wrote: > nit: a const& enum is kinda silly. Done. https://codereview.chromium.org/906493004/diff/200001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/906493004/diff/200001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:138: if (script_->routing_info().render_view_id == routing_id && On 2015/02/23 20:15:00, Devlin wrote: > Should this just work with the injection host call? It seems in current implementation, all webview related logic is handled out of the injection host call. See ProgrammaticScriptInjector::CanExecuteOnFrame(...). The above code grantees that the user script from one webview will only be injected to its corresponding render view. Changes are made to serve this purpose.
https://codereview.chromium.org/906493004/diff/200001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/906493004/diff/200001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:138: if (script_->routing_info().render_view_id == routing_id && On 2015/02/24 16:19:55, Xi Han wrote: > On 2015/02/23 20:15:00, Devlin wrote: > > Should this just work with the injection host call? > > It seems in current implementation, all webview related logic is handled out of > the injection host call. See ProgrammaticScriptInjector::CanExecuteOnFrame(...). > > The above code grantees that the user script from one webview will only be > injected to its corresponding render view. Changes are made to serve this > purpose. While that's true, the reason all webview logic is handled from the script injector, and not the injection host, is because the injection host didn't exist when this was written. :) Looking at the code, it actually looks like we can completely eliminate ScriptInjector::CanExecuteOnFrame in favor of InjectionHost::CanExecuteOnFrame. Let's do that, instead of continuing to have four different places where the "can execute" question is answered. https://codereview.chromium.org/906493004/diff/270001/extensions/renderer/scr... File extensions/renderer/script_injection.h (right): https://codereview.chromium.org/906493004/diff/270001/extensions/renderer/scr... extensions/renderer/script_injection.h:93: When is this used?
Patchset #7 (id:290001) has been deleted
PTAL, thanks! https://codereview.chromium.org/906493004/diff/200001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/906493004/diff/200001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:138: if (script_->routing_info().render_view_id == routing_id && On 2015/02/25 17:14:38, Devlin wrote: > On 2015/02/24 16:19:55, Xi Han wrote: > > On 2015/02/23 20:15:00, Devlin wrote: > > > Should this just work with the injection host call? > > > > It seems in current implementation, all webview related logic is handled out > of > > the injection host call. See > ProgrammaticScriptInjector::CanExecuteOnFrame(...). > > > > The above code grantees that the user script from one webview will only be > > injected to its corresponding render view. Changes are made to serve this > > purpose. > > While that's true, the reason all webview logic is handled from the script > injector, and not the injection host, is because the injection host didn't exist > when this was written. :) Looking at the code, it actually looks like we can > completely eliminate ScriptInjector::CanExecuteOnFrame in favor of > InjectionHost::CanExecuteOnFrame. Let's do that, instead of continuing to have > four different places where the "can execute" question is answered. As discussed offline, leave the ScriptInjector::CanExecuteOnFrame function, but put the script checks below the injection host check. https://codereview.chromium.org/906493004/diff/270001/extensions/renderer/scr... File extensions/renderer/script_injection.h (right): https://codereview.chromium.org/906493004/diff/270001/extensions/renderer/scr... extensions/renderer/script_injection.h:93: On 2015/02/25 17:14:38, Devlin wrote: > When is this used? The get function consumer_instance_type() is used in UserScriptInjector::CanExecuteOnFame() to check the instance type. If it is WEBVIEW, then we will handled differently.
https://codereview.chromium.org/906493004/diff/270001/extensions/renderer/scr... File extensions/renderer/script_injection.h (right): https://codereview.chromium.org/906493004/diff/270001/extensions/renderer/scr... extensions/renderer/script_injection.h:93: On 2015/02/27 19:36:19, Xi Han wrote: > On 2015/02/25 17:14:38, Devlin wrote: > > When is this used? > > The get function consumer_instance_type() is used in > UserScriptInjector::CanExecuteOnFame() to check the instance type. If it is > WEBVIEW, then we will handled differently. Doesn't that check UserScript::consumer_instance_type()? Why does ScriptInjection need it?
Patchset #8 (id:330001) has been deleted
https://codereview.chromium.org/906493004/diff/270001/extensions/renderer/scr... File extensions/renderer/script_injection.h (right): https://codereview.chromium.org/906493004/diff/270001/extensions/renderer/scr... extensions/renderer/script_injection.h:93: On 2015/02/27 21:52:42, Devlin wrote: > On 2015/02/27 19:36:19, Xi Han wrote: > > On 2015/02/25 17:14:38, Devlin wrote: > > > When is this used? > > > > The get function consumer_instance_type() is used in > > UserScriptInjector::CanExecuteOnFame() to check the instance type. If it is > > WEBVIEW, then we will handled differently. > > Doesn't that check UserScript::consumer_instance_type()? Why does > ScriptInjection need it? Oh, you are absolutely right. I took it as the one in UserScript. Removed.
https://codereview.chromium.org/906493004/diff/350001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/906493004/diff/350001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:137: UserScript::ConsumerInstanceType::WEBVIEW || This seems very wrong. Why are we automatically denying all extension injections here?
https://codereview.chromium.org/906493004/diff/350001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/906493004/diff/350001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:137: UserScript::ConsumerInstanceType::WEBVIEW || On 2015/03/02 20:22:42, Devlin wrote: > This seems very wrong. Why are we automatically denying all extension > injections here? Hmm? We didn't deny all extensions injection. The logic here is: If it is not WEBVIEW, we will return the can_execute directly here. If it is WEBVIEW and the can_execute is DENIED, we return directly as well. Only for webview and the host is allowed, we do the following check.
lgtm https://codereview.chromium.org/906493004/diff/350001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/906493004/diff/350001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:137: UserScript::ConsumerInstanceType::WEBVIEW || On 2015/03/02 20:30:17, Xi Han wrote: > On 2015/03/02 20:22:42, Devlin wrote: > > This seems very wrong. Why are we automatically denying all extension > > injections here? > > Hmm? We didn't deny all extensions injection. The logic here is: > If it is not WEBVIEW, we will return the can_execute directly here. If it is > WEBVIEW and the can_execute is DENIED, we return directly as well. Only for > webview and the host is allowed, we do the following check. Huh. I have no idea how I read that so wrong (for some reason in my mind, I saw return ACCESS_DENIED). Nevermind.
The CQ bit was checked by hanxi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/906493004/350001
Message was sent while issue was closed.
Committed patchset #8 (id:350001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/f853287c95cf818d0b5b02d7f4a6588761096d1c Cr-Commit-Position: refs/heads/master@{#318774}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:350001) has been created in https://codereview.chromium.org/972863002/ by cpu@chromium.org. The reason for reverting is: broke the extension tests: http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29... .
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/906493004/#ps370001 (title: "resubmit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/906493004/370001
On 2015/03/03 21:38:19, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/906493004/370001 Did you determine that it wasn't this CL causing the failures?
Message was sent while issue was closed.
Committed patchset #9 (id:370001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/ccff496a48cdf435d59ef79f3fe9ae1e6914ed22 Cr-Commit-Position: refs/heads/master@{#318959}
Message was sent while issue was closed.
On 2015/03/03 21:42:54, Devlin wrote: > On 2015/03/03 21:38:19, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/patch-status/906493004/370001 > > Did you determine that it wasn't this CL causing the failures? fwiw, this cl is causing a bunch of deterministic redness on the clang/win bots, see https://code.google.com/p/chromium/issues/detail?id=463906 . these bots are still in bring-up and less reliable than other bots and redness there could be a compiler bug, but if this got reverted before that's kind of suspicious.
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/906493004/diff/370001/extensions/common/user_... File extensions/common/user_script.h (right): https://codereview.chromium.org/906493004/diff/370001/extensions/common/user_... extensions/common/user_script.h:132: RoutingInfo() : render_process_id(-1), render_view_id(-1) {}; nit: no ';' after method bodies https://codereview.chromium.org/906493004/diff/370001/extensions/common/user_... extensions/common/user_script.h:133: ~RoutingInfo() {}; likewise
Message was sent while issue was closed.
https://codereview.chromium.org/906493004/diff/370001/extensions/browser/user... File extensions/browser/user_script_loader.cc (right): https://codereview.chromium.org/906493004/diff/370001/extensions/browser/user... extensions/browser/user_script_loader.cc:447: // If user scripts are comming from a <webview>, will only notify the nit: "coming" https://codereview.chromium.org/906493004/diff/370001/extensions/common/user_... File extensions/common/user_script.h (right): https://codereview.chromium.org/906493004/diff/370001/extensions/common/user_... extensions/common/user_script.h:316: RoutingInfo routing_info_; You don't initialize this in the constructor, which causes brokenness. Please fix. Initializing this to TAB fixes the clang/win issues. Also consider covering some of this logic by unit tests. DrMemory didn't find this because this apparently is only covered by browser_tests, which seem to not run on DrMemory (probably too slow). I should probably write a clang diag that warns on some fields not getting set
Message was sent while issue was closed.
https://codereview.chromium.org/906493004/diff/370001/extensions/common/user_... File extensions/common/user_script.h (right): https://codereview.chromium.org/906493004/diff/370001/extensions/common/user_... extensions/common/user_script.h:316: RoutingInfo routing_info_; On 2015/03/05 00:40:11, Nico wrote: > You don't initialize this in the constructor, which causes brokenness. Please > fix. Initializing this to TAB fixes the clang/win issues. > > Also consider covering some of this logic by unit tests. > > DrMemory didn't find this because this apparently is only covered by > browser_tests, which seem to not run on DrMemory (probably too slow). > > I should probably write a clang diag that warns on some fields not getting set This comment refers to the variable on line 313. :)
Message was sent while issue was closed.
https://codereview.chromium.org/906493004/diff/370001/extensions/common/user_... File extensions/common/user_script.h (right): https://codereview.chromium.org/906493004/diff/370001/extensions/common/user_... extensions/common/user_script.h:316: RoutingInfo routing_info_; On 2015/03/05 00:42:49, Devlin wrote: > On 2015/03/05 00:40:11, Nico wrote: > > You don't initialize this in the constructor, which causes brokenness. Please > > fix. Initializing this to TAB fixes the clang/win issues. > > > > Also consider covering some of this logic by unit tests. > > > > DrMemory didn't find this because this apparently is only covered by > > browser_tests, which seem to not run on DrMemory (probably too slow). > > > > I should probably write a clang diag that warns on some fields not getting set > > > This comment refers to the variable on line 313. :) Duh, yes, sorry :-( |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
