Add "SHOW ALL SAVED PAGES" button to offline error page

Add "SHOW ALL SAVED PAGES" button to offline error page

BUG=491352
Committed: https://crrev.com/6aabf843cd095e658636f191a9959d1185d45b64
Cr-Commit-Position: refs/heads/master@{#359205}

[jianli, 2015-11-04 01:42:15]

Patchset #1 (id:1) has been deleted

[jianli, 2015-11-04 01:42:22]

Patchset #1 (id:20001) has been deleted

[jianli, 2015-11-05 09:22:07]

jianli@chromium.org changed reviewers:
+ mmenke@chromium.org, newt@chromium.org, rkaplow@chromium.org

[jianli, 2015-11-05 09:22:08]

mmenke for error page changes
newt for android changes
rkaplow for UMA

[mmenke, 2015-11-05 15:44:26]

[+edwardjung]:  FYI.

[mmenke, 2015-11-05 15:55:50]

https://codereview.chromium.org/1410343007/diff/40001/components/error_page/r...
File components/error_page/renderer/net_error_helper_core.cc (right):

https://codereview.chromium.org/1410343007/diff/40001/components/error_page/r...
components/error_page/renderer/net_error_helper_core.cc:602:
RecordEvent(NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN);
This changes the meaning of the old histograms significantly, and is confusing. 
What exactly is the intention here?

https://codereview.chromium.org/1410343007/diff/40001/components/error_page/r...
components/error_page/renderer/net_error_helper_core.cc:948:
RecordEvent(NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN_SAVED_PAGES_CLICKED);
This seems wrong.

[mmenke, 2015-11-05 16:18:12]

https://codereview.chromium.org/1410343007/diff/40001/chrome/common/localized...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/1410343007/diff/40001/chrome/common/localized...
chrome/common/localized_error.cc:771: if (has_offline_pages) {
One other question:  Should we do this for all errors, or only ones that
indicate we may be offline (ERR_INTERNET_DISCONNECTED is pretty certain, but
then can throw in a could others, where we may just have local connectivity. 
ERR_NAME_NOT_RESOLVED, especially).

[edwardjung, 2015-11-05 17:08:01]

edwardjung@chromium.org changed reviewers:
+ edwardjung@chromium.org

[edwardjung, 2015-11-05 17:08:01]

https://codereview.chromium.org/1410343007/diff/40001/chrome/common/localized...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/1410343007/diff/40001/chrome/common/localized...
chrome/common/localized_error.cc:771: if (has_offline_pages) {
On 2015/11/05 16:18:12, mmenke wrote:
> One other question:  Should we do this for all errors, or only ones that
> indicate we may be offline (ERR_INTERNET_DISCONNECTED is pretty certain, but
> then can throw in a could others, where we may just have local connectivity. 
> ERR_NAME_NOT_RESOLVED, especially).

My feeling is that this should be restricted to the offline ones. For some net
errors, a reload might fix the issue. I've seen a proposed layout for how this
looks and it uses the blue button. We generally don't want to the "Show all
saved pages" action to be over an action which might fix the connection issue.  

Selectively showing this link might cause other issues if user learns to expect
to see this button on all net errors. Users will probably not understand our
'offline' logic.

[jianli, 2015-11-06 00:04:41]

https://codereview.chromium.org/1410343007/diff/40001/chrome/common/localized...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/1410343007/diff/40001/chrome/common/localized...
chrome/common/localized_error.cc:771: if (has_offline_pages) {
On 2015/11/05 17:08:01, edwardjung wrote:
> On 2015/11/05 16:18:12, mmenke wrote:
> > One other question:  Should we do this for all errors, or only ones that
> > indicate we may be offline (ERR_INTERNET_DISCONNECTED is pretty certain, but
> > then can throw in a could others, where we may just have local connectivity.

> > ERR_NAME_NOT_RESOLVED, especially).
> 
> My feeling is that this should be restricted to the offline ones. For some net
> errors, a reload might fix the issue. I've seen a proposed layout for how this
> looks and it uses the blue button. We generally don't want to the "Show all
> saved pages" action to be over an action which might fix the connection issue.
 
> 
> Selectively showing this link might cause other issues if user learns to
expect
> to see this button on all net errors. Users will probably not understand our
> 'offline' logic. 

Yes, we only want to offer "Show all saved pages" button in the network offline
scenario which is probably the only case that the user cannot do more than
browsing the previously snapshot of certain pages.

https://codereview.chromium.org/1410343007/diff/40001/components/error_page/r...
File components/error_page/renderer/net_error_helper_core.cc (right):

https://codereview.chromium.org/1410343007/diff/40001/components/error_page/r...
components/error_page/renderer/net_error_helper_core.cc:602:
RecordEvent(NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN);
On 2015/11/05 15:55:50, mmenke wrote:
> This changes the meaning of the old histograms significantly, and is
confusing. 
> What exactly is the intention here?

I am not changing the meaning of the old histograms, but indeed we're adding
some new events to this to accommodate the new cases we have now. Now we're
showing "Show saved pages" button. So I need to track
NETWORK_ERROR_PAGE_SHOW_SAVED_PAGES_BUTTON_SHOWN. Also since "Show saved pages"
button could also appear together with "Show saved copies" button, I want to
track NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN when this is the case.

https://codereview.chromium.org/1410343007/diff/40001/components/error_page/r...
components/error_page/renderer/net_error_helper_core.cc:948:
RecordEvent(NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN_SAVED_PAGES_CLICKED);
On 2015/11/05 15:55:50, mmenke wrote:
> This seems wrong.

Fixed.

[mmenke, 2015-11-06 17:43:02]

https://codereview.chromium.org/1410343007/diff/40001/components/error_page/r...
File components/error_page/renderer/net_error_helper_core.cc (right):

https://codereview.chromium.org/1410343007/diff/40001/components/error_page/r...
components/error_page/renderer/net_error_helper_core.cc:602:
RecordEvent(NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN);
On 2015/11/06 00:04:40, jianli wrote:
> On 2015/11/05 15:55:50, mmenke wrote:
> > This changes the meaning of the old histograms significantly, and is
> confusing. 
> > What exactly is the intention here?
> 
> I am not changing the meaning of the old histograms, but indeed we're adding
> some new events to this to accommodate the new cases we have now. Now we're
> showing "Show saved pages" button. So I need to track
> NETWORK_ERROR_PAGE_SHOW_SAVED_PAGES_BUTTON_SHOWN. Also since "Show saved
pages"
> button could also appear together with "Show saved copies" button, I want to
> track NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN when this is the case.

You are absolutely changing the meaning of
NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN.

It used to be that with
NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN_SAVED_COPY_CLICKED,
NETWORK_ERROR_PAGE_BOTH_BUTTONS_RELOAD_CLICKED, and
NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN, you could tell how often the reload vs
the show save pages button were pressed, when they were both shown.

With your modification, it's impossible to get this value.  In fact, I can't see
how any of the "BUTTONS_SHOWN" numbers could be reasonably interpreted after
this lands.

If show saved pages and show saved copy had the same number of clicks when
"both" buttons were shown, what would that mean?  It could be one was shown 5
times more often than the other, and more rarely clicked, for instance.

[jianli, 2015-11-09 19:19:09]

https://codereview.chromium.org/1410343007/diff/40001/components/error_page/r...
File components/error_page/renderer/net_error_helper_core.cc (right):

https://codereview.chromium.org/1410343007/diff/40001/components/error_page/r...
components/error_page/renderer/net_error_helper_core.cc:602:
RecordEvent(NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN);
On 2015/11/06 17:43:02, mmenke wrote:
> On 2015/11/06 00:04:40, jianli wrote:
> > On 2015/11/05 15:55:50, mmenke wrote:
> > > This changes the meaning of the old histograms significantly, and is
> > confusing. 
> > > What exactly is the intention here?
> > 
> > I am not changing the meaning of the old histograms, but indeed we're adding
> > some new events to this to accommodate the new cases we have now. Now we're
> > showing "Show saved pages" button. So I need to track
> > NETWORK_ERROR_PAGE_SHOW_SAVED_PAGES_BUTTON_SHOWN. Also since "Show saved
> pages"
> > button could also appear together with "Show saved copies" button, I want to
> > track NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN when this is the case.
> 
> You are absolutely changing the meaning of
> NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN.
> 
> It used to be that with
> NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN_SAVED_COPY_CLICKED,
> NETWORK_ERROR_PAGE_BOTH_BUTTONS_RELOAD_CLICKED, and
> NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN, you could tell how often the reload vs
> the show save pages button were pressed, when they were both shown.
> 
> With your modification, it's impossible to get this value.  In fact, I can't
see
> how any of the "BUTTONS_SHOWN" numbers could be reasonably interpreted after
> this lands.
> 
> If show saved pages and show saved copy had the same number of clicks when
> "both" buttons were shown, what would that mean?  It could be one was shown 5
> times more often than the other, and more rarely clicked, for instance.

Thanks for explanation. I reverted to the previous code. Since it is not very
likely to have both saved pages button and saved copy button turned on at the
same time, I think recording just saved pages action should be enough.

[mmenke, 2015-11-09 19:31:44]

LGTM, modulo comments.

https://codereview.chromium.org/1410343007/diff/80001/chrome/browser/net/net_...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://codereview.chromium.org/1410343007/diff/80001/chrome/browser/net/net_...
chrome/browser/net/net_error_tab_helper.cc:126: #if defined(OS_ANDROID)
Should we use a different label for this, or add a comment to all ifdefs in this
CL " // offline pages"?  Think since these are all related, they should all be
easily greppable together.

https://codereview.chromium.org/1410343007/diff/80001/components/error_page/r...
File components/error_page/renderer/net_error_helper_core.cc (right):

https://codereview.chromium.org/1410343007/diff/80001/components/error_page/r...
components/error_page/renderer/net_error_helper_core.cc:948: }
Still need to remove this if.

[jianli, 2015-11-09 21:59:52]

https://codereview.chromium.org/1410343007/diff/80001/chrome/browser/net/net_...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://codereview.chromium.org/1410343007/diff/80001/chrome/browser/net/net_...
chrome/browser/net/net_error_tab_helper.cc:126: #if defined(OS_ANDROID)
On 2015/11/09 19:31:44, mmenke wrote:
> Should we use a different label for this, or add a comment to all ifdefs in
this
> CL " // offline pages"?  Think since these are all related, they should all be
> easily greppable together.

Moved this part into a helper function that is placed together with another
offline pages function.

https://codereview.chromium.org/1410343007/diff/80001/components/error_page/r...
File components/error_page/renderer/net_error_helper_core.cc (right):

https://codereview.chromium.org/1410343007/diff/80001/components/error_page/r...
components/error_page/renderer/net_error_helper_core.cc:948: }
On 2015/11/09 19:31:44, mmenke wrote:
> Still need to remove this if.

Done.

[newt (away), 2015-11-09 22:15:13]

https://codereview.chromium.org/1410343007/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1410343007/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:8797: +        <message
name="IDS_ERRORPAGES_BUTTON_SHOW_SAVED_PAGES" desc="Label for the button on an
error page to show saved pages">
I think you should mention "offline" in the description, e.g. "Label for the
button to show pages that have been saved offline."

https://codereview.chromium.org/1410343007/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:8800: +        <message
name="IDS_ERRORPAGES_BUTTON_SHOW_SAVED_PAGES_HELP" desc="Mobile: Explanation for
the BUTTON_SHOW_SAVED_PAGES button">
Why "Mobile:" here and not on the string above? I'd just remove "Mobile:" unless
there's an equivalent desktop string that we're differentiating this string
against.

Also, referring to a string ID in the description isn't helpful since
translators don't see the string IDs. I'd say "Explanation for the button to
show pages that have been saved offline."

https://codereview.chromium.org/1410343007/diff/100001/chrome/browser/net/net...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://codereview.chromium.org/1410343007/diff/100001/chrome/browser/net/net...
chrome/browser/net/net_error_tab_helper.cc:284: void
NetErrorTabHelper::SetHasOfflinePages(
Does this get called on every page load, or only when an error page is showing?
If it's the former, it seems rather wasteful; seems like we could just query
this state once when we're showing the error page.

[jianli, 2015-11-09 23:08:56]

https://codereview.chromium.org/1410343007/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1410343007/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:8797: +        <message
name="IDS_ERRORPAGES_BUTTON_SHOW_SAVED_PAGES" desc="Label for the button on an
error page to show saved pages">
On 2015/11/09 22:15:13, newt wrote:
> I think you should mention "offline" in the description, e.g. "Label for the
> button to show pages that have been saved offline."

Done.

https://codereview.chromium.org/1410343007/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:8800: +        <message
name="IDS_ERRORPAGES_BUTTON_SHOW_SAVED_PAGES_HELP" desc="Mobile: Explanation for
the BUTTON_SHOW_SAVED_PAGES button">
On 2015/11/09 22:15:12, newt wrote:
> Why "Mobile:" here and not on the string above? I'd just remove "Mobile:"
unless
> there's an equivalent desktop string that we're differentiating this string
> against.
> 
> Also, referring to a string ID in the description isn't helpful since
> translators don't see the string IDs. I'd say "Explanation for the button to
> show pages that have been saved offline."

Done.

https://codereview.chromium.org/1410343007/diff/100001/chrome/browser/net/net...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://codereview.chromium.org/1410343007/diff/100001/chrome/browser/net/net...
chrome/browser/net/net_error_tab_helper.cc:284: void
NetErrorTabHelper::SetHasOfflinePages(
On 2015/11/09 22:15:13, newt wrote:
> Does this get called on every page load, or only when an error page is
showing?
> If it's the former, it seems rather wasteful; seems like we could just query
> this state once when we're showing the error page.

Good catch. Added is_error_page_ check to bail out if not an error page.

[mmenke, 2015-11-09 23:11:47]

MAy want to think about integration tests here - without them, concerned the
wiring between processes could regress.

https://codereview.chromium.org/1410343007/diff/100001/chrome/browser/net/net...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://codereview.chromium.org/1410343007/diff/100001/chrome/browser/net/net...
chrome/browser/net/net_error_tab_helper.cc:284: void
NetErrorTabHelper::SetHasOfflinePages(
On 2015/11/09 23:08:56, jianli wrote:
> On 2015/11/09 22:15:13, newt wrote:
> > Does this get called on every page load, or only when an error page is
> showing?
> > If it's the former, it seems rather wasteful; seems like we could just query
> > this state once when we're showing the error page.
> 
> Good catch. Added is_error_page_ check to bail out if not an error page.

Hrm...Does that actually work?  Won't the message reach the renderer too late?

[jianli, 2015-11-10 00:14:26]

https://codereview.chromium.org/1410343007/diff/100001/chrome/browser/net/net...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://codereview.chromium.org/1410343007/diff/100001/chrome/browser/net/net...
chrome/browser/net/net_error_tab_helper.cc:284: void
NetErrorTabHelper::SetHasOfflinePages(
On 2015/11/09 23:11:47, mmenke wrote:
> On 2015/11/09 23:08:56, jianli wrote:
> > On 2015/11/09 22:15:13, newt wrote:
> > > Does this get called on every page load, or only when an error page is
> > showing?
> > > If it's the former, it seems rather wasteful; seems like we could just
query
> > > this state once when we're showing the error page.
> > 
> > Good catch. Added is_error_page_ check to bail out if not an error page.
> 
> Hrm...Does that actually work?  Won't the message reach the renderer too late?


You're right. |is_error_page_| is only set when the error page is being
reloaded. I also tried to set it in DidFailProvisionalLoad, but it was too late.
The error page setup is only done at DidStartProvisionalLoadForFrame stage. So
we will live with calling this on every page load based on existing
infrastructure.

[newt (away), 2015-11-10 18:27:33]

lgtm after comment about combining strings

https://codereview.chromium.org/1410343007/diff/140001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1410343007/diff/140001/chrome/app/generated_r...
chrome/app/generated_resources.grd:8801: +          Show all saved pages
Because these strings are identical in English, they'll only be translated once
in each language. Hence the translations will be identical. What's the point of
having two strings? Can we just combine them into one?

[rkaplow, 2015-11-10 18:51:23]

lgtm

histograms lgtm

[jianli, 2015-11-10 21:53:12]

jianli@chromium.org changed reviewers:
+ thakis@chromium.org

[jianli, 2015-11-10 21:53:13]

thakis, can you approve chrome changes?

https://codereview.chromium.org/1410343007/diff/140001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1410343007/diff/140001/chrome/app/generated_r...
chrome/app/generated_resources.grd:8801: +          Show all saved pages
On 2015/11/10 18:27:33, newt wrote:
> Because these strings are identical in English, they'll only be translated
once
> in each language. Hence the translations will be identical. What's the point
of
> having two strings? Can we just combine them into one?

Yes, I don't think we need two.

[jianli, 2015-11-11 00:42:30]

jianli@chromium.org changed reviewers:
+ thestig@chromium.org
- thakis@chromium.org

[jianli, 2015-11-11 00:42:31]

thestig, could you please review and approve common changes in chrome? thanks.

[Lei Zhang, 2015-11-11 00:55:58]

lgtm for chrome/ code that's non-android and non-net.

https://codereview.chromium.org/1410343007/diff/160001/chrome/browser/net/net...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://codereview.chromium.org/1410343007/diff/160001/chrome/browser/net/net...
chrome/browser/net/net_error_tab_helper.cc:288: web_contents() ?
TabAndroid::FromWebContents(web_contents()) : nullptr;
The rest of the files doesn't seem to care if web_contents() returns a nullptr.
Can you check the lifetime of this class and see if web_contents() is always
valid?

https://codereview.chromium.org/1410343007/diff/160001/chrome/common/render_m...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/1410343007/diff/160001/chrome/common/render_m...
chrome/common/render_messages.h:327: // Tells the renderer whether or not an
offline page exists. This is used to
You need an IPC reviewer anyway, but I hope that person agrees with me in
telling you this should be #ifdef'd. Assuming that's the case, then #ifdef the
code that uses this IPC accordingly.

[jianli, 2015-11-11 01:16:47]

https://codereview.chromium.org/1410343007/diff/160001/chrome/browser/net/net...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://codereview.chromium.org/1410343007/diff/160001/chrome/browser/net/net...
chrome/browser/net/net_error_tab_helper.cc:288: web_contents() ?
TabAndroid::FromWebContents(web_contents()) : nullptr;
On 2015/11/11 00:55:57, Lei Zhang wrote:
> The rest of the files doesn't seem to care if web_contents() returns a
nullptr.
> Can you check the lifetime of this class and see if web_contents() is always
> valid?

Done.

https://codereview.chromium.org/1410343007/diff/160001/chrome/common/render_m...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/1410343007/diff/160001/chrome/common/render_m...
chrome/common/render_messages.h:327: // Tells the renderer whether or not an
offline page exists. This is used to
On 2015/11/11 00:55:57, Lei Zhang wrote:
> You need an IPC reviewer anyway, but I hope that person agrees with me in
> telling you this should be #ifdef'd. Assuming that's the case, then #ifdef the
> code that uses this IPC accordingly.

Done.

[jianli, 2015-11-11 01:17:04]

jianli@chromium.org changed reviewers:
+ dcheng@chromium.org

[jianli, 2015-11-11 01:17:05]

dcheng, could you please reveiw new IPC?

[jianli, 2015-11-11 01:18:40]

jianli@chromium.org changed reviewers:
+ dimich@chromium.org

[jianli, 2015-11-11 01:18:40]

dimich, could you please review offline_pages DEP added to chrome/common/DEPS?
thanks.

[dcheng, 2015-11-11 20:19:09]

ipc changes lgtm

[jianli, 2015-11-11 22:07:48]

The CQ bit was checked by jianli@chromium.org

[jianli, 2015-11-11 22:07:48]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
rkaplow@chromium.org, newt@chromium.org, thestig@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1410343007/#ps220001
(title: "Remove unneeded dep")

[commit-bot: I haz the power, 2015-11-11 22:09:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1410343007/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1410343007/220001

[commit-bot: I haz the power, 2015-11-11 22:13:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-11 22:13:03]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fgorski, 2015-11-11 22:16:13]

On 2015/11/11 22:13:03, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
>   mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

Please double check the indexes on the histogram

[jianli, 2015-11-11 22:19:51]

On 2015/11/11 22:16:13, fgorski wrote:
> On 2015/11/11 22:13:03, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
> >   mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
> >   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
> 
> Please double check the indexes on the histogram

Good catch. Someone forgot to update the histogram last time. Fixed.

[jianli, 2015-11-11 22:30:16]

The CQ bit was checked by jianli@chromium.org

[jianli, 2015-11-11 22:30:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, newt@chromium.org, thestig@chromium.org,
dcheng@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1410343007/#ps260001
(title: "Rebase")

[commit-bot: I haz the power, 2015-11-11 22:37:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1410343007/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1410343007/260001

[commit-bot: I haz the power, 2015-11-11 23:02:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-11 23:02:56]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[jianli, 2015-11-11 23:14:07]

The CQ bit was checked by jianli@chromium.org

[jianli, 2015-11-11 23:14:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, newt@chromium.org, thestig@chromium.org,
dcheng@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1410343007/#ps280001
(title: "Fix after rebase")

[commit-bot: I haz the power, 2015-11-11 23:15:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1410343007/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1410343007/280001

[commit-bot: I haz the power, 2015-11-12 01:17:50]

Committed patchset #13 (id:280001)

[commit-bot: I haz the power, 2015-11-12 20:02:39]

Patchset 13 (id:??) landed as
https://crrev.com/6aabf843cd095e658636f191a9959d1185d45b64
Cr-Commit-Position: refs/heads/master@{#359205}