Load live version when reloading an offline page on connected network

Load live version when reloading an offline page on connected network

In order to trigger reloading currently-displayed as offline page with
network condition check, we resend the request with same extra request
header with reason field updated.

BUG=644943
Committed: https://crrev.com/f68c52f414b7b785720faf53458df2517e921c9e
Cr-Commit-Position: refs/heads/master@{#419868}

[jianli, 2016-09-14 01:44:43]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 01:45:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jianli, 2016-09-14 01:45:45]

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

[commit-bot: I haz the power, 2016-09-14 01:58:46]

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

[commit-bot: I haz the power, 2016-09-14 01:58:46]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[jianli, 2016-09-16 20:50:34]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 20:51:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 20:54:42]

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

[commit-bot: I haz the power, 2016-09-16 20:54:43]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Dmitry Titov, 2016-09-16 21:37:24]

https://codereview.chromium.org/2337363002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
(right):

https://codereview.chromium.org/2337363002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:218:
foundTab.reload();
Please add comment why simple reload will do the right thing here, which is not
really a reload in some cases.

https://codereview.chromium.org/2337363002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_request_job.cc (right):

https://codereview.chromium.org/2337363002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:143: // If
offline header contains a reason other than RELOAD, the offline page
Not sure I understand this. Reason also includes UNKNOWN for example...
Would it be better to check for equality with DOWNLOAD and NET_ERROR? That would
feel like more specific check.

https://codereview.chromium.org/2337363002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right):

https://codereview.chromium.org/2337363002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:46:
offline_header_.Clear();
after rebase, you will need to do figure out when to save/set the header as I
did with provisional_offline_page_...

https://codereview.chromium.org/2337363002/diff/1/chrome/browser/android/tab_...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2337363002/diff/1/chrome/browser/android/tab_...
chrome/browser/android/tab_android.cc:851:
offline_pages::OfflinePageHeader::Reason::RELOAD;
Please add comment why do you need the rest of the header here (instead of just
creating a new one with reason=reload).

https://codereview.chromium.org/2337363002/diff/1/components/offline_pages/re...
File components/offline_pages/request_header/offline_page_header.h (right):

https://codereview.chromium.org/2337363002/diff/1/components/offline_pages/re...
components/offline_pages/request_header/offline_page_header.h:25: extern const
char kOfflinePageHeaderReasonValueFromDownload[];
These two values shoudl be commented.

https://codereview.chromium.org/2337363002/diff/1/components/offline_pages/re...
components/offline_pages/request_header/offline_page_header.h:47: bool
ParseFromHeaderValue(const std::string& header_value);
Why not just have an explicit ctor from a string? (in addition to a default one)

https://codereview.chromium.org/2337363002/diff/1/components/offline_pages/re...
components/offline_pages/request_header/offline_page_header.h:51: std::string
ToFullHeaderString() const;
GetCompleteHeaderString ?

[jianli, 2016-09-16 23:08:30]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[jianli, 2016-09-16 23:08:48]

https://codereview.chromium.org/2337363002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
(right):

https://codereview.chromium.org/2337363002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:218:
foundTab.reload();
On 2016/09/16 21:37:23, Dmitry Titov wrote:
> Please add comment why simple reload will do the right thing here, which is
not
> really a reload in some cases.

Done.

https://codereview.chromium.org/2337363002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_request_job.cc (right):

https://codereview.chromium.org/2337363002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:143: // If
offline header contains a reason other than RELOAD, the offline page
On 2016/09/16 21:37:23, Dmitry Titov wrote:
> Not sure I understand this. Reason also includes UNKNOWN for example...
> Would it be better to check for equality with DOWNLOAD and NET_ERROR? That
would
> feel like more specific check.

I removed UNKNOWN value. So all values except RELOAD should mean to always the
offline page.

https://codereview.chromium.org/2337363002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right):

https://codereview.chromium.org/2337363002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:46:
offline_header_.Clear();
On 2016/09/16 21:37:23, Dmitry Titov wrote:
> after rebase, you will need to do figure out when to save/set the header as I
> did with provisional_offline_page_...

Yes. I think we should also avoid clearing is_offline_preview_.

Fixed.

https://codereview.chromium.org/2337363002/diff/1/chrome/browser/android/tab_...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2337363002/diff/1/chrome/browser/android/tab_...
chrome/browser/android/tab_android.cc:851:
offline_pages::OfflinePageHeader::Reason::RELOAD;
On 2016/09/16 21:37:23, Dmitry Titov wrote:
> Please add comment why do you need the rest of the header here (instead of
just
> creating a new one with reason=reload).

Done.

https://codereview.chromium.org/2337363002/diff/1/components/offline_pages/re...
File components/offline_pages/request_header/offline_page_header.h (right):

https://codereview.chromium.org/2337363002/diff/1/components/offline_pages/re...
components/offline_pages/request_header/offline_page_header.h:25: extern const
char kOfflinePageHeaderReasonValueFromDownload[];
On 2016/09/16 21:37:23, Dmitry Titov wrote:
> These two values shoudl be commented.

Done.

https://codereview.chromium.org/2337363002/diff/1/components/offline_pages/re...
components/offline_pages/request_header/offline_page_header.h:47: bool
ParseFromHeaderValue(const std::string& header_value);
On 2016/09/16 21:37:23, Dmitry Titov wrote:
> Why not just have an explicit ctor from a string? (in addition to a default
one)

If we use an explicit ctor from a string, we need to expose something like
parsed_successfully(). For the case that default ctor is used, returning either
true or false for parsed_successfully is kind of confusing. We can expose
another state or change this to tri-state, but it will make the whole logic more
complicated. So I think introducing an explicit parsing method seems to be more
straightforward.

https://codereview.chromium.org/2337363002/diff/1/components/offline_pages/re...
components/offline_pages/request_header/offline_page_header.h:51: std::string
ToFullHeaderString() const;
On 2016/09/16 21:37:23, Dmitry Titov wrote:
> GetCompleteHeaderString ?

Done.

[commit-bot: I haz the power, 2016-09-16 23:08:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 23:11:57]

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

[commit-bot: I haz the power, 2016-09-16 23:11:58]

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

[jianli, 2016-09-16 23:17:35]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 23:18:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jianli, 2016-09-16 23:26:41]

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

[jianli, 2016-09-16 23:26:42]

Ted, could you please review android tab changes? Thanks.

[jianli, 2016-09-16 23:28:22]

Description was changed from

==========
Load live version when reloading an offline page on connected network

BUG=644943
==========

to

==========
Load live version when reloading an offline page on connected network

In order to trigger reloading currently-displayed as offline page with network
condition check, we resend the request with same extra request header with
reason field updated.

BUG=644943
==========

[jianli, 2016-09-16 23:28:42]

Description was changed from

==========
Load live version when reloading an offline page on connected network

In order to trigger reloading currently-displayed as offline page with network
condition check, we resend the request with same extra request header with
reason field updated.

BUG=644943
==========

to

==========
Load live version when reloading an offline page on connected network

In order to trigger reloading currently-displayed as offline page with
network condition check, we resend the request with same extra request
header with reason field updated.

BUG=644943
==========

[commit-bot: I haz the power, 2016-09-16 23:46:14]

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

[commit-bot: I haz the power, 2016-09-16 23:46:17]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[jianli, 2016-09-17 01:54:25]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-17 01:54:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dmitry Titov, 2016-09-17 02:22:54]

Great, getting close!

https://codereview.chromium.org/2337363002/diff/80001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right):

https://codereview.chromium.org/2337363002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:44: if
(navigation_handle->IsSynchronousNavigation())
As discussed, lets keep the IsSamePage() check in DidFinishNamvigation, and not
use IsSynchronousNavigation. 

So I think the best approach here is:
0. Have provisional values for page, header, is_offline_preview.
1. Clear provisional values in DidStartNavigation
2. Most of the time (but not always) Interceptor will chime in with
SetOfflinePage and set provisional values
3. In DidFinishNavigation, if not IsSamePage, do real_values <-
provisional-values.

This works in many combinations when some callbacks are missing/repeated, and is
more direct.

https://codereview.chromium.org/2337363002/diff/100001/components/offline_pag...
File components/offline_pages/request_header/offline_page_header.h (right):

https://codereview.chromium.org/2337363002/diff/100001/components/offline_pag...
components/offline_pages/request_header/offline_page_header.h:53: // Constructed
from a request header value string.
Please add comment here that if parse fails it's equivalent to calling Clear().

https://codereview.chromium.org/2337363002/diff/100001/components/offline_pag...
components/offline_pages/request_header/offline_page_header.h:65: bool is_valid;
This is only useful for tests, to see if parsing failed. Can be named as
did_fail_parsing_for_test, for clarity.
Also, should not reset to false in Clear()... Once failed, should always be
true.

You Clear() on failed parse, which is perfect reaction to failure (invalid
header ignored), so this bool is not needed in the rest of the code.

[commit-bot: I haz the power, 2016-09-17 03:03:56]

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

[commit-bot: I haz the power, 2016-09-17 03:03:56]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[jianli, 2016-09-19 21:28:56]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[jianli, 2016-09-19 21:29:05]

https://codereview.chromium.org/2337363002/diff/80001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right):

https://codereview.chromium.org/2337363002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:44: if
(navigation_handle->IsSynchronousNavigation())
On 2016/09/17 02:22:54, Dmitry Titov wrote:
> As discussed, lets keep the IsSamePage() check in DidFinishNamvigation, and
not
> use IsSynchronousNavigation. 
> 
> So I think the best approach here is:
> 0. Have provisional values for page, header, is_offline_preview.
> 1. Clear provisional values in DidStartNavigation

We also need to clear the real values here because otherwise the following bug
will be hit:
1) Open a downloaded page on connected network. The offline indicator should be
shown.
2) Manually type any other URL of a web site that could take a while to load.
The offline indicator will not be reset during loading.

> 2. Most of the time (but not always) Interceptor will chime in with
> SetOfflinePage and set provisional values
> 3. In DidFinishNavigation, if not IsSamePage, do real_values <-
> provisional-values.
> 
> This works in many combinations when some callbacks are missing/repeated, and
is
> more direct.

https://codereview.chromium.org/2337363002/diff/100001/components/offline_pag...
File components/offline_pages/request_header/offline_page_header.h (right):

https://codereview.chromium.org/2337363002/diff/100001/components/offline_pag...
components/offline_pages/request_header/offline_page_header.h:53: // Constructed
from a request header value string.
On 2016/09/17 02:22:54, Dmitry Titov wrote:
> Please add comment here that if parse fails it's equivalent to calling
Clear().

Done.

https://codereview.chromium.org/2337363002/diff/100001/components/offline_pag...
components/offline_pages/request_header/offline_page_header.h:65: bool is_valid;
On 2016/09/17 02:22:54, Dmitry Titov wrote:
> This is only useful for tests, to see if parsing failed. Can be named as
> did_fail_parsing_for_test, for clarity.
> Also, should not reset to false in Clear()... Once failed, should always be
> true.
> 
> You Clear() on failed parse, which is perfect reaction to failure (invalid
> header ignored), so this bool is not needed in the rest of the code.

Done.

[commit-bot: I haz the power, 2016-09-19 21:29:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ted C, 2016-09-19 21:53:49]

https://codereview.chromium.org/2337363002/diff/120001/chrome/browser/android...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2337363002/diff/120001/chrome/browser/android...
chrome/browser/android/tab_android.cc:841: ScopedJavaLocalRef<jstring>
TabAndroid::GetOfflinePageHeaderForReload(
Do we need to add these to tab_android?  Could we not instead add them to
OfflinePageUtils in java (and then have those call into the native offline page
utils).

Ideally, we can isolate all offline logic into offline page files to avid
muddling the meaning of this class (and these functions all seem to be
relatively simple pass throughs to the C++ implementation).

[jianli, 2016-09-19 21:54:49]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 21:55:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dmitry Titov, 2016-09-19 22:06:13]

https://codereview.chromium.org/2337363002/diff/140001/chrome/browser/android...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2337363002/diff/140001/chrome/browser/android...
chrome/browser/android/tab_android.cc:822: const offline_pages::OfflinePageItem*
offline_page =
This is unreachable code :) It might be not needed?

https://codereview.chromium.org/2337363002/diff/140001/chrome/browser/android...
chrome/browser/android/tab_android.cc:841: ScopedJavaLocalRef<jstring>
TabAndroid::GetOfflinePageHeaderForReload(
+1 to what Ted said on moving this into OfflinePageUtil on java side...

[jianli, 2016-09-19 22:26:06]

https://codereview.chromium.org/2337363002/diff/120001/chrome/browser/android...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2337363002/diff/120001/chrome/browser/android...
chrome/browser/android/tab_android.cc:841: ScopedJavaLocalRef<jstring>
TabAndroid::GetOfflinePageHeaderForReload(
On 2016/09/19 21:53:49, Ted C wrote:
> Do we need to add these to tab_android?  Could we not instead add them to
> OfflinePageUtils in java (and then have those call into the native offline
page
> utils).
> 
> Ideally, we can isolate all offline logic into offline page files to avid
> muddling the meaning of this class (and these functions all seem to be
> relatively simple pass throughs to the C++ implementation).

Done.

https://codereview.chromium.org/2337363002/diff/140001/chrome/browser/android...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2337363002/diff/140001/chrome/browser/android...
chrome/browser/android/tab_android.cc:822: const offline_pages::OfflinePageItem*
offline_page =
On 2016/09/19 22:06:13, Dmitry Titov wrote:
> This is unreachable code :) It might be not needed?

Removed line 820-821.

https://codereview.chromium.org/2337363002/diff/140001/chrome/browser/android...
chrome/browser/android/tab_android.cc:841: ScopedJavaLocalRef<jstring>
TabAndroid::GetOfflinePageHeaderForReload(
On 2016/09/19 22:06:13, Dmitry Titov wrote:
> +1 to what Ted said on moving this into OfflinePageUtil on java side...

Done.

[jianli, 2016-09-19 22:27:07]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 22:27:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jianli, 2016-09-19 23:51:05]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 23:51:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 01:11:26]

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

[commit-bot: I haz the power, 2016-09-20 01:11:27]

Dry run: This issue passed the CQ dry run.

[Dmitry Titov, 2016-09-20 18:41:15]

lgtm

[Ted C, 2016-09-20 19:22:27]

On 2016/09/20 18:41:15, Dmitry Titov wrote:
> lgtm

Tab.java lgtm

As a followup, let's try to remove the remaining offline calls from tab_android
native as well.  Also, why not make OfflinePageUtils directly talk to
offline_page_utils instead of going through the bridge?  That seems unnecessary
indirection.

[jianli, 2016-09-20 20:07:49]

On Tue, Sep 20, 2016 at 12:22 PM, <tedchoc@chromium.org> wrote:

> On 2016/09/20 18:41:15, Dmitry Titov wrote:
> > lgtm
>
> Tab.java lgtm
>
> As a followup, let's try to remove the remaining offline calls from
> tab_android
> native as well. Also, why not make OfflinePageUtils directly talk to
> offline_page_utils instead of going through the bridge? That seems
> unnecessary
> indirection.
>

Agreed. We're going to do a refectoring on this on separate patches.
Thanks.

>
>
> https://codereview.chromium.org/2337363002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jianli, 2016-09-20 20:07:50]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2016-09-20 20:08:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 20:18:19]

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

[commit-bot: I haz the power, 2016-09-20 20:18:21]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[brettw, 2016-09-20 20:39:14]

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

[brettw, 2016-09-20 20:39:16]

owners lgtm with line deleted

https://codereview.chromium.org/2337363002/diff/180001/components/offline_pag...
File components/offline_pages/request_header/BUILD.gn (right):

https://codereview.chromium.org/2337363002/diff/180001/components/offline_pag...
components/offline_pages/request_header/BUILD.gn:9: # GYP:
//components/offline_pages.gypi:request_header
Why did you add this? offline_pages.gypi doesn't even exist.

[jianli, 2016-09-20 20:46:54]

https://codereview.chromium.org/2337363002/diff/180001/components/offline_pag...
File components/offline_pages/request_header/BUILD.gn (right):

https://codereview.chromium.org/2337363002/diff/180001/components/offline_pag...
components/offline_pages/request_header/BUILD.gn:9: # GYP:
//components/offline_pages.gypi:request_header
On 2016/09/20 20:39:15, brettw (ping on IM after 24h) wrote:
> Why did you add this? offline_pages.gypi doesn't even exist.

Done.

[jianli, 2016-09-20 20:47:33]

The CQ bit was checked by jianli@chromium.org

[jianli, 2016-09-20 20:47:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, brettw@chromium.org, dimich@chromium.org
Link to the patchset: https://codereview.chromium.org/2337363002/#ps200001
(title: "Update BUILD.gn")

[commit-bot: I haz the power, 2016-09-20 20:48:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 22:10:25]

Description was changed from

==========
Load live version when reloading an offline page on connected network

In order to trigger reloading currently-displayed as offline page with
network condition check, we resend the request with same extra request
header with reason field updated.

BUG=644943
==========

to

==========
Load live version when reloading an offline page on connected network

In order to trigger reloading currently-displayed as offline page with
network condition check, we resend the request with same extra request
header with reason field updated.

BUG=644943
==========

[commit-bot: I haz the power, 2016-09-20 22:10:26]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-09-20 22:11:53]

Description was changed from

==========
Load live version when reloading an offline page on connected network

In order to trigger reloading currently-displayed as offline page with
network condition check, we resend the request with same extra request
header with reason field updated.

BUG=644943
==========

to

==========
Load live version when reloading an offline page on connected network

In order to trigger reloading currently-displayed as offline page with
network condition check, we resend the request with same extra request
header with reason field updated.

BUG=644943

Committed: https://crrev.com/f68c52f414b7b785720faf53458df2517e921c9e
Cr-Commit-Position: refs/heads/master@{#419868}
==========

[commit-bot: I haz the power, 2016-09-20 22:11:55]

Patchset 11 (id:??) landed as
https://crrev.com/f68c52f414b7b785720faf53458df2517e921c9e
Cr-Commit-Position: refs/heads/master@{#419868}