Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1129353005:
Move Google Cache copy suggestion to an action button in the neterror interstitial (Closed)
|
Created:
5 years, 7 months ago by edwardjung Modified:
5 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove Google Cache copy suggestion to an action button in the neterror interstitial
+ Replaces the reload button
+ Removes the suggestion text from under the details section
BUG=474848
Committed: https://crrev.com/0478c03f9462d8c543c0dddd1a034d35b1a85fc3
Cr-Commit-Position: refs/heads/master@{#333731}
Patch Set 1 #Patch Set 2 : Add finch experiment code #
Total comments: 20
Patch Set 3 : Address review comments #
Total comments: 2
Patch Set 4 : #
Total comments: 19
Patch Set 5 : Review comments #
Total comments: 10
Patch Set 6 : Address review comments #Patch Set 7 : Fix size_t to int data loss warning #
Messages
Total messages: 40 (9 generated)
edwardjung@chromium.org changed reviewers: + felt@chromium.org
Felt could you have a look at this code for an experiment on the net error page. I wasn't sure if it was better to replace the reload button or to add another button to the template. Thanks.
https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... chrome/common/localized_error.cc:676: std::string fieldTrialExpType = base::FieldTrialList::FindFullName( in C++ style this should be field_trial_exp_type https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... chrome/common/localized_error.cc:681: if (!suggestions->empty() && fieldTrialExpType != "") { !field_trial_exp_type.empty() https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... chrome/common/localized_error.cc:681: if (!suggestions->empty() && fieldTrialExpType != "") { Do you also want to support a control group (e.g., the field trial will respond with "control") for the experiment? If so the check should probably go up here. https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... chrome/common/localized_error.cc:695: if (fieldTrialExpType == "copy") { I'd recommend setting both "EnableGoogleCachedCopyTextExperiment" and "copy" as constants in an anonymous namespace at the top. It makes it easier to look the constants up quickly or change them later. https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... chrome/common/localized_error.cc:697: "msg", l10n_util::GetStringUTF16( nit: this line wrap (and the one below) looks kinda wonky. did `git cl format` do it this way? https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... chrome/common/localized_error.cc:703: } Is there another condition besides "copy"? How might you land in this branch?
swapping out the button content seems OK https://codereview.chromium.org/1129353005/diff/20001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/1129353005/diff/20001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:123: reloadButton.onclick = ''; should this be removeEventListener? or are you stuck doing onclick = '' because the other function isn't named?
Addressed your comments. Regarding the control, if this is just the same state as currently I should still define a control set right in the experiment right? I've not set the finch config yet. https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... chrome/common/localized_error.cc:676: std::string fieldTrialExpType = base::FieldTrialList::FindFullName( On 2015/05/11 20:46:44, felt wrote: > in C++ style this should be field_trial_exp_type Done. https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... chrome/common/localized_error.cc:681: if (!suggestions->empty() && fieldTrialExpType != "") { On 2015/05/11 20:46:43, felt wrote: > !field_trial_exp_type.empty() Done. https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... chrome/common/localized_error.cc:681: if (!suggestions->empty() && fieldTrialExpType != "") { On 2015/05/11 20:46:44, felt wrote: > Do you also want to support a control group (e.g., the field trial will respond > with "control") for the experiment? If so the check should probably go up here. Good point, will add a control. https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... chrome/common/localized_error.cc:695: if (fieldTrialExpType == "copy") { On 2015/05/11 20:46:43, felt wrote: > I'd recommend setting both "EnableGoogleCachedCopyTextExperiment" and "copy" as > constants in an anonymous namespace at the top. It makes it easier to look the > constants up quickly or change them later. Done. https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... chrome/common/localized_error.cc:697: "msg", l10n_util::GetStringUTF16( On 2015/05/11 20:46:43, felt wrote: > nit: this line wrap (and the one below) looks kinda wonky. did `git cl format` > do it this way? I wasn't even aware of that command. Great time saver. https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... chrome/common/localized_error.cc:703: } On 2015/05/11 20:46:43, felt wrote: > Is there another condition besides "copy"? How might you land in this branch? My thinking is there would be three sets: + "control" - as it is now, no change. + "copy" - Button with "Show cached copy" + "page" - Button with "Show cached page" Should I specify these conditions. https://codereview.chromium.org/1129353005/diff/20001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/1129353005/diff/20001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:123: reloadButton.onclick = ''; On 2015/05/11 20:49:01, felt wrote: > should this be removeEventListener? or are you stuck doing onclick = '' because > the other function isn't named? removeEventListener only works on events registered with addEventListener. I actually realised this doesn't work properly and should have used removeAttribute instead.
https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... chrome/common/localized_error.cc:676: std::string fieldTrialExpType = base::FieldTrialList::FindFullName( On 2015/05/12 10:16:50, edwardjung wrote: > On 2015/05/11 20:46:44, felt wrote: > > in C++ style this should be field_trial_exp_type > > Done. still seems to be field_trial_exp_type in the latest patchset https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... chrome/common/localized_error.cc:697: "msg", l10n_util::GetStringUTF16( On 2015/05/12 10:16:50, edwardjung wrote: > On 2015/05/11 20:46:43, felt wrote: > > nit: this line wrap (and the one below) looks kinda wonky. did `git cl format` > > do it this way? > > I wasn't even aware of that command. Great time saver. always double check it, sometimes it does weird stuff. but it is useful. :) https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... chrome/common/localized_error.cc:703: } On 2015/05/12 10:16:50, edwardjung wrote: > On 2015/05/11 20:46:43, felt wrote: > > Is there another condition besides "copy"? How might you land in this branch? > > My thinking is there would be three sets: > + "control" - as it is now, no change. > + "copy" - Button with "Show cached copy" > + "page" - Button with "Show cached page" > > Should I specify these conditions. I think it's clear now in the newest version. https://codereview.chromium.org/1129353005/diff/40001/chrome/common/localized... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/1129353005/diff/40001/chrome/common/localized... chrome/common/localized_error.cc:705: } else if (fieldTrialExpType == kCachedCopyButtonExpTypePage) { You might have had the right idea with the else branch before, since now if the Finch code returns something unexpected this code won't set anything for "msg". A few ideas: * an else branch that DCHECKs * an else branch with a default msg * move the "page" condition back into a plain else branch but with a comment saying it is for "page"
On 2015/05/12 10:16:50, edwardjung wrote: > Addressed your comments. > > Regarding the control, if this is just the same state as currently I should > still define a control set right in the experiment right? I've not set the finch > config yet. Yes, you'll still want to define a control set (with the current behavior) in the Finch config. > > https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... > File chrome/common/localized_error.cc (right): > > https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... > chrome/common/localized_error.cc:676: std::string fieldTrialExpType = > base::FieldTrialList::FindFullName( > On 2015/05/11 20:46:44, felt wrote: > > in C++ style this should be field_trial_exp_type > > Done. > > https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... > chrome/common/localized_error.cc:681: if (!suggestions->empty() && > fieldTrialExpType != "") { > On 2015/05/11 20:46:43, felt wrote: > > !field_trial_exp_type.empty() > > Done. > > https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... > chrome/common/localized_error.cc:681: if (!suggestions->empty() && > fieldTrialExpType != "") { > On 2015/05/11 20:46:44, felt wrote: > > Do you also want to support a control group (e.g., the field trial will > respond > > with "control") for the experiment? If so the check should probably go up > here. > > Good point, will add a control. > > https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... > chrome/common/localized_error.cc:695: if (fieldTrialExpType == "copy") { > On 2015/05/11 20:46:43, felt wrote: > > I'd recommend setting both "EnableGoogleCachedCopyTextExperiment" and "copy" > as > > constants in an anonymous namespace at the top. It makes it easier to look the > > constants up quickly or change them later. > > Done. > > https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... > chrome/common/localized_error.cc:697: "msg", l10n_util::GetStringUTF16( > On 2015/05/11 20:46:43, felt wrote: > > nit: this line wrap (and the one below) looks kinda wonky. did `git cl format` > > do it this way? > > I wasn't even aware of that command. Great time saver. > > https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... > chrome/common/localized_error.cc:703: } > On 2015/05/11 20:46:43, felt wrote: > > Is there another condition besides "copy"? How might you land in this branch? > > My thinking is there would be three sets: > + "control" - as it is now, no change. > + "copy" - Button with "Show cached copy" > + "page" - Button with "Show cached page" > > Should I specify these conditions. > > https://codereview.chromium.org/1129353005/diff/20001/chrome/renderer/resourc... > File chrome/renderer/resources/neterror.js (right): > > https://codereview.chromium.org/1129353005/diff/20001/chrome/renderer/resourc... > chrome/renderer/resources/neterror.js:123: reloadButton.onclick = ''; > On 2015/05/11 20:49:01, felt wrote: > > should this be removeEventListener? or are you stuck doing onclick = '' > because > > the other function isn't named? > > removeEventListener only works on events registered with addEventListener. I > actually realised this doesn't work properly and should have used > removeAttribute instead.
Thanks. https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... chrome/common/localized_error.cc:676: std::string fieldTrialExpType = base::FieldTrialList::FindFullName( On 2015/05/12 14:15:08, felt wrote: > On 2015/05/12 10:16:50, edwardjung wrote: > > On 2015/05/11 20:46:44, felt wrote: > > > in C++ style this should be field_trial_exp_type > > > > Done. > > still seems to be field_trial_exp_type in the latest patchset Should it be field_type_exp_type or fieldTrialExpType? I moved it to a the kCachedCopyButtonFieldTrial const at the top. https://codereview.chromium.org/1129353005/diff/40001/chrome/common/localized... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/1129353005/diff/40001/chrome/common/localized... chrome/common/localized_error.cc:705: } else if (fieldTrialExpType == kCachedCopyButtonExpTypePage) { Reverted back to using the else statement
edwardjung@chromium.org changed reviewers: + arv@chromium.org, mmenke@chromium.org, sky@chromium.org
Hi all, please take a look at the respected changes: mmenke: components/error_page/renderer/net_error_helper_core.cc sky: chrome/common/localized_error.cc arv: chrome/renderer/resources/neterror.js chrome/renderer/resources/neterror.css Thanks.
https://codereview.chromium.org/1129353005/diff/60001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/1129353005/diff/60001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:123: reloadButton.url = buttonStrings.cacheUrl; No need to add expandos here and below. https://codereview.chromium.org/1129353005/diff/60001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:124: reloadButton.trackingid = buttonStrings.trackingId; trackingid -> trackingId https://codereview.chromium.org/1129353005/diff/60001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:125: reloadButton.removeAttribute('onclick'); Why? Instead of using addEventListener on the next line you can just assign to onclick reloadButton.onclick = function(e) { and it will replace the existing one. https://codereview.chromium.org/1129353005/diff/60001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:127: e.preventDefault(); wrong indentation https://codereview.chromium.org/1129353005/diff/60001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:131: }); OK. Here is what you will end up with: var url = buttonStrings.cacheUrl; var trackingid = buttonStrings.trackingId; reloadButton.onclick = function(e) { e.preventDefault(); trackClick(trackingId); location = url; };
https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... chrome/common/localized_error.cc:676: std::string fieldTrialExpType = base::FieldTrialList::FindFullName( On 2015/05/13 09:59:56, edwardjung wrote: > On 2015/05/12 14:15:08, felt wrote: > > On 2015/05/12 10:16:50, edwardjung wrote: > > > On 2015/05/11 20:46:44, felt wrote: > > > > in C++ style this should be field_trial_exp_type > > > > > > Done. > > > > still seems to be field_trial_exp_type in the latest patchset > > Should it be field_type_exp_type or fieldTrialExpType? I moved it to a the > kCachedCopyButtonFieldTrial const at the top. field_type_exp_type all local variables are done with underscores in C++, sorry for my confusing comment
components/error_page LGTM. https://codereview.chromium.org/1129353005/diff/60001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/1129353005/diff/60001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:119: function setupCachedButton(buttonStrings) { nit: setUp ("setup" is a noun, want "set up" here)
Are field trials loaded in the renderer? https://codereview.chromium.org/1129353005/diff/60001/chrome/common/localized... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/1129353005/diff/60001/chrome/common/localized... chrome/common/localized_error.cc:678: suggestions = params->override_suggestions.release(); How about moving this block to it's own function? https://codereview.chromium.org/1129353005/diff/60001/chrome/common/localized... chrome/common/localized_error.cc:681: std::string fieldTrialExpType = field_trial_exp_type. https://codereview.chromium.org/1129353005/diff/60001/chrome/common/localized... chrome/common/localized_error.cc:689: suggestions->GetDictionary(0, &suggestion); Don't you need to check the return value here? https://codereview.chromium.org/1129353005/diff/60001/chrome/common/localized... chrome/common/localized_error.cc:699: base::DictionaryValue* cache_button = new base::DictionaryValue; used scoped_ptr and release when passing to Set below.
Thanks all. New patchset uploaded. https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/1129353005/diff/20001/chrome/common/localized... chrome/common/localized_error.cc:676: std::string fieldTrialExpType = base::FieldTrialList::FindFullName( > > field_type_exp_type all local variables are done with underscores in C++, sorry > for my confusing comment No, my fault, couldn't see the wood for the trees. https://codereview.chromium.org/1129353005/diff/60001/chrome/common/localized... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/1129353005/diff/60001/chrome/common/localized... chrome/common/localized_error.cc:678: suggestions = params->override_suggestions.release(); On 2015/05/13 15:52:03, sky wrote: > How about moving this block to it's own function? Done. https://codereview.chromium.org/1129353005/diff/60001/chrome/common/localized... chrome/common/localized_error.cc:681: std::string fieldTrialExpType = On 2015/05/13 15:52:03, sky wrote: > field_trial_exp_type. Done. https://codereview.chromium.org/1129353005/diff/60001/chrome/common/localized... chrome/common/localized_error.cc:689: suggestions->GetDictionary(0, &suggestion); On 2015/05/13 15:52:03, sky wrote: > Don't you need to check the return value here? Yes, I do via DictionaryValue suggestion. https://codereview.chromium.org/1129353005/diff/60001/chrome/common/localized... chrome/common/localized_error.cc:699: base::DictionaryValue* cache_button = new base::DictionaryValue; On 2015/05/13 15:52:03, sky wrote: > used scoped_ptr and release when passing to Set below. Done. https://codereview.chromium.org/1129353005/diff/60001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/1129353005/diff/60001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:119: function setupCachedButton(buttonStrings) { On 2015/05/13 14:58:40, mmenke wrote: > nit: setUp ("setup" is a noun, want "set up" here) Done. https://codereview.chromium.org/1129353005/diff/60001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:124: reloadButton.trackingid = buttonStrings.trackingId; On 2015/05/13 14:26:22, arv wrote: > trackingid -> trackingId Done. https://codereview.chromium.org/1129353005/diff/60001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:125: reloadButton.removeAttribute('onclick'); On 2015/05/13 14:26:22, arv wrote: > Why? Instead of using addEventListener on the next line you can just assign to > onclick > > reloadButton.onclick = function(e) { > > and it will replace the existing one. Good point. https://codereview.chromium.org/1129353005/diff/60001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:127: e.preventDefault(); On 2015/05/13 14:26:22, arv wrote: > wrong indentation Done. https://codereview.chromium.org/1129353005/diff/60001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:131: }); On 2015/05/13 14:26:22, arv wrote: > OK. Here is what you will end up with: > > var url = buttonStrings.cacheUrl; > var trackingid = buttonStrings.trackingId; > reloadButton.onclick = function(e) { > e.preventDefault(); > trackClick(trackingId); > location = url; > }; Done.
renderer/resources/* LGTM
> Are field trials loaded in the renderer? motek confirmed that recent changes means that field trials should work in renderer. I'm not passing experiment parameters, which currently don't work in the renderer. Thanks.
On 2015/05/20 13:25:48, edwardjung wrote: > > Are field trials loaded in the renderer? > > motek confirmed that recent changes means that field trials should work in > renderer. I'm not passing experiment parameters, which currently don't work in > the renderer. > > Thanks. Have you tested this? You can test field trials with a command line flag: --force-fieldtrials=ExperimentName/ConditionName/
> Have you tested this? You can test field trials with a command line flag: > --force-fieldtrials=ExperimentName/ConditionName/ Yep. Tested that and it works fine.
Sky, friendly ping, could you take a look. Thanks,
Sorry, what files do you need me to review?
> Sorry, what files do you need me to review? chrome/common/localized_error.cc chrome/common/localized_error.h Thanks
https://codereview.chromium.org/1129353005/diff/80001/chrome/common/localized... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/1129353005/diff/80001/chrome/common/localized... chrome/common/localized_error.cc:526: void LocalizedError::EnableGoogleCachedCopyButtonExperiment( Make position match that of header. https://codereview.chromium.org/1129353005/diff/80001/chrome/common/localized... chrome/common/localized_error.cc:541: if (type == 0) { where does 0 come from? Please use a constant. https://codereview.chromium.org/1129353005/diff/80001/chrome/common/localized... chrome/common/localized_error.cc:559: cache_button->SetString("cacheUrl", cache_url); You're not checking the url is valid. Should you? https://codereview.chromium.org/1129353005/diff/80001/chrome/common/localized... chrome/common/localized_error.cc:562: suggestions->Remove(0, NULL); nullptr https://codereview.chromium.org/1129353005/diff/80001/chrome/common/localized... chrome/common/localized_error.cc:562: suggestions->Remove(0, NULL); Why do you need to remove?
Sky, I've addressed your comments, please take another look. Thanks. https://codereview.chromium.org/1129353005/diff/80001/chrome/common/localized... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/1129353005/diff/80001/chrome/common/localized... chrome/common/localized_error.cc:526: void LocalizedError::EnableGoogleCachedCopyButtonExperiment( On 2015/06/02 16:18:06, sky wrote: > Make position match that of header. Done. https://codereview.chromium.org/1129353005/diff/80001/chrome/common/localized... chrome/common/localized_error.cc:541: if (type == 0) { On 2015/06/02 16:18:06, sky wrote: > where does 0 come from? Please use a constant. Done. https://codereview.chromium.org/1129353005/diff/80001/chrome/common/localized... chrome/common/localized_error.cc:559: cache_button->SetString("cacheUrl", cache_url); On 2015/06/02 16:18:06, sky wrote: > You're not checking the url is valid. Should you? This link is returned from the link doctor so I would assume the link would be valid. https://codereview.chromium.org/1129353005/diff/80001/chrome/common/localized... chrome/common/localized_error.cc:562: suggestions->Remove(0, NULL); On 2015/06/02 16:18:06, sky wrote: > Why do you need to remove? As the suggestion text would be displayed in the details section, which we don't want. https://codereview.chromium.org/1129353005/diff/80001/chrome/common/localized... chrome/common/localized_error.cc:562: suggestions->Remove(0, NULL); On 2015/06/02 16:18:06, sky wrote: > nullptr Done.
Ping! On 2015/06/03 13:40:37, edwardjung wrote: > Sky, I've addressed your comments, please take another look. Thanks. > > https://codereview.chromium.org/1129353005/diff/80001/chrome/common/localized... > File chrome/common/localized_error.cc (right): > > https://codereview.chromium.org/1129353005/diff/80001/chrome/common/localized... > chrome/common/localized_error.cc:526: void > LocalizedError::EnableGoogleCachedCopyButtonExperiment( > On 2015/06/02 16:18:06, sky wrote: > > Make position match that of header. > > Done. > > https://codereview.chromium.org/1129353005/diff/80001/chrome/common/localized... > chrome/common/localized_error.cc:541: if (type == 0) { > On 2015/06/02 16:18:06, sky wrote: > > where does 0 come from? Please use a constant. > > Done. > > https://codereview.chromium.org/1129353005/diff/80001/chrome/common/localized... > chrome/common/localized_error.cc:559: cache_button->SetString("cacheUrl", > cache_url); > On 2015/06/02 16:18:06, sky wrote: > > You're not checking the url is valid. Should you? > > This link is returned from the link doctor so I would assume the link would be > valid. > > https://codereview.chromium.org/1129353005/diff/80001/chrome/common/localized... > chrome/common/localized_error.cc:562: suggestions->Remove(0, NULL); > On 2015/06/02 16:18:06, sky wrote: > > Why do you need to remove? > > As the suggestion text would be displayed in the details section, which we don't > want. > > https://codereview.chromium.org/1129353005/diff/80001/chrome/common/localized... > chrome/common/localized_error.cc:562: suggestions->Remove(0, NULL); > On 2015/06/02 16:18:06, sky wrote: > > nullptr > > Done.
LGTM
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, arv@chromium.org Link to the patchset: https://codereview.chromium.org/1129353005/#ps100001 (title: "Address review comments")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129353005/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org, sky@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1129353005/#ps120001 (title: "Fix size_t to int data loss warning")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129353005/120001
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 edwardjung@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129353005/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0478c03f9462d8c543c0dddd1a034d35b1a85fc3 Cr-Commit-Position: refs/heads/master@{#333731} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
