Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2655823002:
Include referrer chain with inline install requests. (Closed)
|
Created:
3 years, 11 months ago by robertshield Modified:
3 years, 10 months ago Reviewers:
Devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInclude referrer chain with inline install requests.
BUG=685905
Review-Url: https://codereview.chromium.org/2655823002
Cr-Commit-Position: refs/heads/master@{#447515}
Committed: https://chromium.googlesource.com/chromium/src/+/1fc1a4a7f6d66e0e8afad18258fc478e9b151d59
Patch Set 1 #Patch Set 2 : Whitespace. #Patch Set 3 : Add browser_test. #Patch Set 4 : cleanup #
Total comments: 18
Patch Set 5 : Devlin feedback. #Patch Set 6 : Cleanup. #
Total comments: 11
Patch Set 7 : Devlin comments. #Patch Set 8 : Add missing comment. #Messages
Total messages: 36 (25 generated)
The CQ bit was checked by robertshield@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robertshield@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Include referrer chain with inline install requests. BUG=<need to open a bug> ========== to ========== Include referrer chain with inline install requests. BUG=685905 ==========
robertshield@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Forked and edited from https://codereview.chromium.org/2466263010/. PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice! https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/webstore_data_fetcher.cc (right): https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/webstore_data_fetcher.cc:41: json_post_data_.swap(*json); If we always dereference this, why pass it as a unique ptr? https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/webstore_inline_installer.cc (right): https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer.cc:104: void WebstoreInlineInstaller::StartWebstoreDataRequest( In the original version of this patch, this used a async history method to get the redirect chain, so this separation was more necessary. Now, this is all sync (and using the navigation entry seems more correct anyway). With this, I think we could clean this up a bit. What if instead of having OnBeforeWebstoreDataRequest() which by default calls fetcher->Start() we just have a method like virtual std::string GetJsonPostData() which by default returns an empty string, and we override it here to return the redirect chain? Then we just call that from the base class and assign the post data if it's non-empty. That'd get rid of some of the bouncing around here. https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer.cc:113: for (const GURL& url : redirect_list) { Are we worried at all about the size of the redirect chain? This could be a large number of large urls... https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right): https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:35: using net::test_server::HttpRequest; nit: avoid using 'using' for single uses. :) https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:418: for (auto redirect : redirects) { const auto& https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:419: std::string redir = base::StringPrintf("http://%s:%d/server-redirect?", maybe s/redir/url or redirect_url? https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:434: EXPECT_EQ(3, cws_request_redirects_); It'd be nice to verify the contents, too. Maybe just store the DictionaryValue and check the items? https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/webstore_installer_test.cc (right): https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/webstore_installer_test.cc:61: &WebstoreInstallerTest::MonitorServerRequest, base::Unretained(this))); Why not bind to ProcessServerRequest directly? https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/webstore_standalone_installer.cc (right): https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/webstore_standalone_installer.cc:72: OnBeforeWebstoreDataRequest(webstore_data_fetcher_.get()); (Following up from the other comment) So here, we'd just do something like: std::string json_data = GetJsonPostData(); if (!json_data.empty()) webstore_data_fetcher_->SetJsonPostData(json_data); webstore_data_fetcher_->Start();
The CQ bit was checked by robertshield@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by robertshield@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Devlin, PTAL! https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/webstore_data_fetcher.cc (right): https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/webstore_data_fetcher.cc:41: json_post_data_.swap(*json); On 2017/01/27 20:31:34, Devlin (catching up) wrote: > If we always dereference this, why pass it as a unique ptr? Indeed, simplified. https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/webstore_inline_installer.cc (right): https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer.cc:104: void WebstoreInlineInstaller::StartWebstoreDataRequest( On 2017/01/27 20:31:34, Devlin (catching up) wrote: > In the original version of this patch, this used a async history method to get > the redirect chain, so this separation was more necessary. Now, this is all > sync (and using the navigation entry seems more correct anyway). With this, I > think we could clean this up a bit. > > What if instead of having OnBeforeWebstoreDataRequest() which by default calls > fetcher->Start() we just have a method like > virtual std::string GetJsonPostData() > which by default returns an empty string, and we override it here to return the > redirect chain? Then we just call that from the base class and assign the post > data if it's non-empty. That'd get rid of some of the bouncing around here. Good idea! That's a lot cleaner and makes deriving from WebstoreStandaloneInstaller much easier. Done. https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer.cc:113: for (const GURL& url : redirect_list) { On 2017/01/27 20:31:34, Devlin (catching up) wrote: > Are we worried at all about the size of the redirect chain? This could be a > large number of large urls... Good question, I'm not really sure. A estimate of an upper bound is ~2K x 20 (max url size x max redirects) or 40K per POST request. I'll see if an extra max 40K of data, probably much less than this most of the time, would be a concern (I suspect not). https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right): https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:35: using net::test_server::HttpRequest; On 2017/01/27 20:31:34, Devlin (catching up) wrote: > nit: avoid using 'using' for single uses. :) Done. https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:418: for (auto redirect : redirects) { On 2017/01/27 20:31:34, Devlin (catching up) wrote: > const auto& Done. https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:419: std::string redir = base::StringPrintf("http://%s:%d/server-redirect?", On 2017/01/27 20:31:34, Devlin (catching up) wrote: > maybe s/redir/url or redirect_url? Done. https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:434: EXPECT_EQ(3, cws_request_redirects_); On 2017/01/27 20:31:34, Devlin (catching up) wrote: > It'd be nice to verify the contents, too. Maybe just store the DictionaryValue > and check the items? Done. https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/webstore_installer_test.cc (right): https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/webstore_installer_test.cc:61: &WebstoreInstallerTest::MonitorServerRequest, base::Unretained(this))); On 2017/01/27 20:31:34, Devlin (catching up) wrote: > Why not bind to ProcessServerRequest directly? Hrmm.. I think I had MonitorServerRequest doing something more interesting in an earlier version. Now cleaned up, thanks. https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/webstore_standalone_installer.cc (right): https://codereview.chromium.org/2655823002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/webstore_standalone_installer.cc:72: OnBeforeWebstoreDataRequest(webstore_data_fetcher_.get()); On 2017/01/27 20:31:34, Devlin (catching up) wrote: > (Following up from the other comment) > > So here, we'd just do something like: > std::string json_data = GetJsonPostData(); > if (!json_data.empty()) > webstore_data_fetcher_->SetJsonPostData(json_data); > webstore_data_fetcher_->Start(); Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice! A few last nits, but looks pretty good overall. https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/webstore_data_fetcher.h (right): https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_data_fetcher.h:11: #include "base/gtest_prod_util.h" needed? https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/webstore_inline_installer.cc (right): https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer.cc:107: if (web_contents) { Can |web_contents| be null? If so, we should maybe document when, because it's not always intuitive. Also, I think this would be more readable as: if (!web_contents) return std::string(); <lots of other stuff> rather than if (web_contents) { <lots of other stuff> } return std::string() https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer.cc:129: if (base::JSONWriter::Write(dictionary, &json)) Could this fail? If this is just to be cautious, maybe use a CHECK() instead so we know if/when it crashes. https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right): https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:391: if (contents->IsType(base::Value::Type::DICTIONARY)) { Would this ever not be the case, especially in a test? Or can we just make this ASSERT_EQ(base::Value::Type::DICTIONARY, contents->type()); https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:431: const std::vector<std::string> expected_redirect_domains{ nitty nit: more common in chromium to use = {}, so expected_redirect_domains = {kRedirect1, kRedirect2, kAppDomain}
https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/webstore_data_fetcher.h (right): https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_data_fetcher.h:11: #include "base/gtest_prod_util.h" On 2017/01/30 17:24:48, Devlin wrote: > needed? Done. https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/webstore_inline_installer.cc (right): https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer.cc:107: if (web_contents) { On 2017/01/30 17:24:48, Devlin wrote: > Can |web_contents| be null? Spelunking a bit: GetWebContents() returns web_contents() (I switched to using web_contents() to be consistent with the rest of this file). web_contents() comes from contents::WebContentsObserver. Its value is stored in WebstoreInlineInstaller's constructor and is passed by WebstoreInlineInstallerFactory::CreateInstaller which gets its WebContents instance from the TabHelper that owns the WebstoreInlineInstaller instance. web_contents_ does get reset if the WebContents instance itself gets destroyed, via a call to contents::WebContentsObserver::ResetWebContents(). So, afaict, web_contents_ should always be non-null except perhaps during TabHelper destruction. This code shouldn't be called destruction now since it's called synchronously from inside a TabContents event handler. It potentially could if this gets refactored into something async (like the first HistoryService-based implementation) at a later date and it's unlikely that would be obvious during a refactor. Given the un-intuitiveness, I'd prefer to leave the check in with a brief comment. Let me know what you think. > > If so, we should maybe document when, because it's not always intuitive. Also, > I think this would be more readable as: > > if (!web_contents) > return std::string(); > > <lots of other stuff> > > rather than > > if (web_contents) { > <lots of other stuff> > } > > return std::string() Done. https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer.cc:129: if (base::JSONWriter::Write(dictionary, &json)) On 2017/01/30 17:24:48, Devlin wrote: > Could this fail? If this is just to be cautious, maybe use a CHECK() instead so > we know if/when it crashes. Actually, Write() always DCHECKs before returning false (which should only happen if the base::Value it gets passed is somehow mangled) so I'll skip the check here. https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right): https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:391: if (contents->IsType(base::Value::Type::DICTIONARY)) { On 2017/01/30 17:24:48, Devlin wrote: > Would this ever not be the case, especially in a test? Or can we just make this > ASSERT_EQ(base::Value::Type::DICTIONARY, contents->type()); Done. https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer_browsertest.cc:431: const std::vector<std::string> expected_redirect_domains{ On 2017/01/30 17:24:48, Devlin wrote: > nitty nit: more common in chromium to use = {}, so > expected_redirect_domains = {kRedirect1, kRedirect2, kAppDomain} Done.
https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/webstore_inline_installer.cc (right): https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_inline_installer.cc:107: if (web_contents) { On 2017/01/31 02:48:14, robertshield_slow_reviews wrote: > On 2017/01/30 17:24:48, Devlin wrote: > > Can |web_contents| be null? > > Spelunking a bit: GetWebContents() returns web_contents() (I switched to using > web_contents() to be consistent with the rest of this file). > > web_contents() comes from contents::WebContentsObserver. Its value is stored in > WebstoreInlineInstaller's constructor and is passed by > WebstoreInlineInstallerFactory::CreateInstaller which gets its WebContents > instance from the TabHelper that owns the WebstoreInlineInstaller instance. > web_contents_ does get reset if the WebContents instance itself gets destroyed, > via a call to contents::WebContentsObserver::ResetWebContents(). > > So, afaict, web_contents_ should always be non-null except perhaps during > TabHelper destruction. > > This code shouldn't be called destruction now since it's called synchronously > from inside a TabContents event handler. > > It potentially could if this gets refactored into something async (like the > first HistoryService-based implementation) at a later date and it's unlikely > that would be obvious during a refactor. > > Given the un-intuitiveness, I'd prefer to leave the check in with a brief > comment. Let me know what you think. > > > > > If so, we should maybe document when, because it's not always intuitive. > Also, > > I think this would be more readable as: > > > > if (!web_contents) > > return std::string(); > > > > <lots of other stuff> > > > > rather than > > > > if (web_contents) { > > <lots of other stuff> > > } > > > > return std::string() > > Done. Actually.. I spelunked some more. Looking at e.g. crash/f397a0e680000000, it looks like there are cases where the other code in this file that does not check for web_contents() being null crashes in the field every once in a while in what I'm guessing is a race against window destruction. I'm going to leave the check in here and if you like, I can remove the DCHECK on line 179 and change it to a run-time check in another CL.
The CQ bit was checked by robertshield@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/31 02:56:28, robertshield_slow_reviews wrote: > https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... > File chrome/browser/extensions/webstore_inline_installer.cc (right): > > https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... > chrome/browser/extensions/webstore_inline_installer.cc:107: if (web_contents) { > On 2017/01/31 02:48:14, robertshield_slow_reviews wrote: > > On 2017/01/30 17:24:48, Devlin wrote: > > > Can |web_contents| be null? > > > > Spelunking a bit: GetWebContents() returns web_contents() (I switched to using > > web_contents() to be consistent with the rest of this file). > > > > web_contents() comes from contents::WebContentsObserver. Its value is stored > in > > WebstoreInlineInstaller's constructor and is passed by > > WebstoreInlineInstallerFactory::CreateInstaller which gets its WebContents > > instance from the TabHelper that owns the WebstoreInlineInstaller instance. > > web_contents_ does get reset if the WebContents instance itself gets > destroyed, > > via a call to contents::WebContentsObserver::ResetWebContents(). > > > > So, afaict, web_contents_ should always be non-null except perhaps during > > TabHelper destruction. > > > > This code shouldn't be called destruction now since it's called synchronously > > from inside a TabContents event handler. > > > > It potentially could if this gets refactored into something async (like the > > first HistoryService-based implementation) at a later date and it's unlikely > > that would be obvious during a refactor. > > > > Given the un-intuitiveness, I'd prefer to leave the check in with a brief > > comment. Let me know what you think. > > > > > > > > If so, we should maybe document when, because it's not always intuitive. > > Also, > > > I think this would be more readable as: > > > > > > if (!web_contents) > > > return std::string(); > > > > > > <lots of other stuff> > > > > > > rather than > > > > > > if (web_contents) { > > > <lots of other stuff> > > > } > > > > > > return std::string() > > > > Done. > > Actually.. I spelunked some more. Looking at e.g. crash/f397a0e680000000, it > looks like there are cases where the other code in this file that does not check > for web_contents() being null crashes in the field every once in a while in what > I'm guessing is a race against window destruction. > > I'm going to leave the check in here and if you like, I can remove the DCHECK on > line 179 and change it to a run-time check in another CL. More spelunking, those crashes might be because web_contents() returns null, or because the tab has been removed from the TabStripModel holding it. Either or, I'd still like to keep the check in this CL and let me know if you'd like a fix for the crash in WebstoreInlineInstaller::CheckInlineInstallPermitted in another CL.
The CQ bit was checked by robertshield@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/31 03:39:55, robertshield_slow_reviews wrote: > On 2017/01/31 02:56:28, robertshield_slow_reviews wrote: > > > https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... > > File chrome/browser/extensions/webstore_inline_installer.cc (right): > > > > > https://codereview.chromium.org/2655823002/diff/100001/chrome/browser/extensi... > > chrome/browser/extensions/webstore_inline_installer.cc:107: if (web_contents) > { > > On 2017/01/31 02:48:14, robertshield_slow_reviews wrote: > > > On 2017/01/30 17:24:48, Devlin wrote: > > > > Can |web_contents| be null? > > > > > > Spelunking a bit: GetWebContents() returns web_contents() (I switched to > using > > > web_contents() to be consistent with the rest of this file). > > > > > > web_contents() comes from contents::WebContentsObserver. Its value is stored > > in > > > WebstoreInlineInstaller's constructor and is passed by > > > WebstoreInlineInstallerFactory::CreateInstaller which gets its WebContents > > > instance from the TabHelper that owns the WebstoreInlineInstaller instance. > > > web_contents_ does get reset if the WebContents instance itself gets > > destroyed, > > > via a call to contents::WebContentsObserver::ResetWebContents(). > > > > > > So, afaict, web_contents_ should always be non-null except perhaps during > > > TabHelper destruction. > > > > > > This code shouldn't be called destruction now since it's called > synchronously > > > from inside a TabContents event handler. > > > > > > It potentially could if this gets refactored into something async (like the > > > first HistoryService-based implementation) at a later date and it's unlikely > > > that would be obvious during a refactor. > > > > > > Given the un-intuitiveness, I'd prefer to leave the check in with a brief > > > comment. Let me know what you think. > > > > > > > > > > > If so, we should maybe document when, because it's not always intuitive. > > > Also, > > > > I think this would be more readable as: > > > > > > > > if (!web_contents) > > > > return std::string(); > > > > > > > > <lots of other stuff> > > > > > > > > rather than > > > > > > > > if (web_contents) { > > > > <lots of other stuff> > > > > } > > > > > > > > return std::string() > > > > > > Done. > > > > Actually.. I spelunked some more. Looking at e.g. crash/f397a0e680000000, it > > looks like there are cases where the other code in this file that does not > check > > for web_contents() being null crashes in the field every once in a while in > what > > I'm guessing is a race against window destruction. > > > > I'm going to leave the check in here and if you like, I can remove the DCHECK > on > > line 179 and change it to a run-time check in another CL. > > More spelunking, those crashes might be because web_contents() returns null, or > because the tab has been removed from the TabStripModel holding it. > > Either or, I'd still like to keep the check in this CL and let me know if you'd > like a fix for the crash in WebstoreInlineInstaller::CheckInlineInstallPermitted > in another CL. Thanks for all the cave-diving! I'm fine with leaving the check in to be on the safe side. We can re-evaluate later if it's provably not a concern.
lgtm
The CQ bit was checked by robertshield@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485959654475420,
"parent_rev": "2cd89ce477eb1e5ec041612369f29d75e651919e", "commit_rev":
"1fc1a4a7f6d66e0e8afad18258fc478e9b151d59"}
Message was sent while issue was closed.
Description was changed from ========== Include referrer chain with inline install requests. BUG=685905 ========== to ========== Include referrer chain with inline install requests. BUG=685905 Review-Url: https://codereview.chromium.org/2655823002 Cr-Commit-Position: refs/heads/master@{#447515} Committed: https://chromium.googlesource.com/chromium/src/+/1fc1a4a7f6d66e0e8afad18258fc... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/1fc1a4a7f6d66e0e8afad18258fc... |
