Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2602953002:
[NTP::RecentTabs] Do not force offline version of the page when open. (Closed)
Description[NTP::RecentTabs] Do not force offline version of the page when open.
When opening a recent tab suggestion, open the tab instead of forcing
offline page. Previously, when user opened a recent tab suggestions,
its offline version was always oppened. After this CL, the tab is simply
opened and it is up to Offline Pages team whether to open its offline
version. This was a PM decision (to provide the same functionality as
tab switcher has).
BUG=674894
Committed: https://crrev.com/ff328670d44ef1bc94a93a6f64657f7be098b22e
Cr-Commit-Position: refs/heads/master@{#441030}
Patch Set 1 #
Total comments: 4
Patch Set 2 : dgn@ nit. #Messages
Total messages: 24 (15 generated)
The CQ bit was checked by vitaliii@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...
vitaliii@chromium.org changed reviewers: + dgn@chromium.org
Hi dgn@, could you have a look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nit in commit description: s/openning/opening lgtm https://codereview.chromium.org/2602953002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2602953002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:414: // The offline version of the page is not forced here. The tab is simply opened. It is I don't think this comment is needed, as we already say so at line 382.
Description was changed from ========== [NTP::RecentTabs] Do not force offline version of the page when open. When openning a recent tab suggestion, open the tab instead of forcing offline page. Previously, when user opened a recent tab suggestions, its offline version was always oppened. After this CL, the tab is simply opened and it is up to Offline Pages team whether to open its offline version. This was a PM decision (to provide the same functionality as tab switcher has). BUG=674894 ========== to ========== [NTP::RecentTabs] Do not force offline version of the page when open. When opening a recent tab suggestion, open the tab instead of forcing offline page. Previously, when user opened a recent tab suggestions, its offline version was always oppened. After this CL, the tab is simply opened and it is up to Offline Pages team whether to open its offline version. This was a PM decision (to provide the same functionality as tab switcher has). BUG=674894 ==========
Also, can you please reflow the CL description to 72 characters?
Description was changed from ========== [NTP::RecentTabs] Do not force offline version of the page when open. When opening a recent tab suggestion, open the tab instead of forcing offline page. Previously, when user opened a recent tab suggestions, its offline version was always oppened. After this CL, the tab is simply opened and it is up to Offline Pages team whether to open its offline version. This was a PM decision (to provide the same functionality as tab switcher has). BUG=674894 ========== to ========== [NTP::RecentTabs] Do not force offline version of the page when open. When opening a recent tab suggestion, open the tab instead of forcing offline page. Previously, when user opened a recent tab suggestions, its offline version was always oppened. After this CL, the tab is simply opened and it is up to Offline Pages team whether to open its offline version. This was a PM decision (to provide the same functionality as tab switcher has). BUG=674894 ==========
Hi dgn@, I reflowed the description and corrected the typo. Also I am not sure about your nit, PTAL at my comment. https://codereview.chromium.org/2602953002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2602953002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:414: // The offline version of the page is not forced here. The tab is simply opened. It is On 2016/12/30 11:38:03, dgn wrote: > I don't think this comment is needed, as we already say so at line 382. This is a bit different. There we just open a URL, while here we switch to a tab. Otherwise, there would be no need to handle recent tabs differently. Also one could get to this point before reaching line 382. I would prefer to leave it, WDYT?
https://codereview.chromium.org/2602953002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2602953002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:414: // The offline version of the page is not forced here. The tab is simply opened. It is On 2016/12/30 11:46:55, vitaliii wrote: > On 2016/12/30 11:38:03, dgn wrote: > > I don't think this comment is needed, as we already say so at line 382. > > This is a bit different. > There we just open a URL, while here we switch to a tab. Otherwise, there would > be no need to handle recent tabs differently. > Also one could get to this point before reaching line 382. > > I would prefer to leave it, WDYT? But that's a tab that already exists, right? and we just switch to it. If the content of the tab was discarded because we ran out of memory or similar, the tab would refresh it, and if we are offline at that point, the offline copy is loaded. But that's all implementation details of how tab switching is done. Did I follow correctly? If so, that's the default behaviour. I'm not a big fan of adding comments to explain why we are not doing things that we would not expect to be doing in the first place. git blame is there to explain why things changed. IMO this comment does not help much when looking at the code purely in its current state. It makes it feel like an exception. The general policy we have with offline is that we directly open offline only in the "downloaded" section, and for the rest we don't care. So that's in line with it.
Description was changed from ========== [NTP::RecentTabs] Do not force offline version of the page when open. When opening a recent tab suggestion, open the tab instead of forcing offline page. Previously, when user opened a recent tab suggestions, its offline version was always oppened. After this CL, the tab is simply opened and it is up to Offline Pages team whether to open its offline version. This was a PM decision (to provide the same functionality as tab switcher has). BUG=674894 ========== to ========== [NTP::RecentTabs] Do not force offline version of the page when open. When opening a recent tab suggestion, open the tab instead of forcing offline page. Previously, when user opened a recent tab suggestions, its offline version was always oppened. After this CL, the tab is simply opened and it is up to Offline Pages team whether to open its offline version. This was a PM decision (to provide the same functionality as tab switcher has). BUG=674894 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel ==========
Patchset #2 (id:20001) has been deleted
Description was changed from ========== [NTP::RecentTabs] Do not force offline version of the page when open. When opening a recent tab suggestion, open the tab instead of forcing offline page. Previously, when user opened a recent tab suggestions, its offline version was always oppened. After this CL, the tab is simply opened and it is up to Offline Pages team whether to open its offline version. This was a PM decision (to provide the same functionality as tab switcher has). BUG=674894 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel ========== to ========== [NTP::RecentTabs] Do not force offline version of the page when open. When opening a recent tab suggestion, open the tab instead of forcing offline page. Previously, when user opened a recent tab suggestions, its offline version was always oppened. After this CL, the tab is simply opened and it is up to Offline Pages team whether to open its offline version. This was a PM decision (to provide the same functionality as tab switcher has). BUG=674894 ==========
Hi dgn@, I addressed your nit, no need to look. https://codereview.chromium.org/2602953002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2602953002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:414: // The offline version of the page is not forced here. The tab is simply opened. It is On 2016/12/30 12:35:42, dgn wrote: > On 2016/12/30 11:46:55, vitaliii wrote: > > On 2016/12/30 11:38:03, dgn wrote: > > > I don't think this comment is needed, as we already say so at line 382. > > > > This is a bit different. > > There we just open a URL, while here we switch to a tab. Otherwise, there > would > > be no need to handle recent tabs differently. > > Also one could get to this point before reaching line 382. > > > > I would prefer to leave it, WDYT? > > But that's a tab that already exists, right? and we just switch to it. If the > content of the tab was discarded because we ran out of memory or similar, the > tab would refresh it, and if we are offline at that point, the offline copy is > loaded. But that's all implementation details of how tab switching is done. Did > I follow correctly? Yes, indeed. > If so, that's the default behaviour. I'm not a big fan of adding comments to > explain why we are not doing things that we would not expect to be doing in the > first place. git blame is there to explain why things changed. IMO this comment > does not help much when looking at the code purely in its current state. It > makes it feel like an exception. The general policy we have with offline is that > we directly open offline only in the "downloaded" section, and for the rest we > don't care. So that's in line with it. Fair enough, I removed the comment.
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org Link to the patchset: https://codereview.chromium.org/2602953002/#ps20002 (title: "dgn@ nit.")
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": 20002, "attempt_start_ts": 1483103304795530,
"parent_rev": "49b9e7554a552e0486a5be082dc15aab1598fc36", "commit_rev":
"6953ce1c2820f1f6e02aeb015af807d23938bb31"}
Message was sent while issue was closed.
Description was changed from ========== [NTP::RecentTabs] Do not force offline version of the page when open. When opening a recent tab suggestion, open the tab instead of forcing offline page. Previously, when user opened a recent tab suggestions, its offline version was always oppened. After this CL, the tab is simply opened and it is up to Offline Pages team whether to open its offline version. This was a PM decision (to provide the same functionality as tab switcher has). BUG=674894 ========== to ========== [NTP::RecentTabs] Do not force offline version of the page when open. When opening a recent tab suggestion, open the tab instead of forcing offline page. Previously, when user opened a recent tab suggestions, its offline version was always oppened. After this CL, the tab is simply opened and it is up to Offline Pages team whether to open its offline version. This was a PM decision (to provide the same functionality as tab switcher has). BUG=674894 Review-Url: https://codereview.chromium.org/2602953002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20002)
Message was sent while issue was closed.
Description was changed from ========== [NTP::RecentTabs] Do not force offline version of the page when open. When opening a recent tab suggestion, open the tab instead of forcing offline page. Previously, when user opened a recent tab suggestions, its offline version was always oppened. After this CL, the tab is simply opened and it is up to Offline Pages team whether to open its offline version. This was a PM decision (to provide the same functionality as tab switcher has). BUG=674894 Review-Url: https://codereview.chromium.org/2602953002 ========== to ========== [NTP::RecentTabs] Do not force offline version of the page when open. When opening a recent tab suggestion, open the tab instead of forcing offline page. Previously, when user opened a recent tab suggestions, its offline version was always oppened. After this CL, the tab is simply opened and it is up to Offline Pages team whether to open its offline version. This was a PM decision (to provide the same functionality as tab switcher has). BUG=674894 Committed: https://crrev.com/ff328670d44ef1bc94a93a6f64657f7be098b22e Cr-Commit-Position: refs/heads/master@{#441030} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ff328670d44ef1bc94a93a6f64657f7be098b22e Cr-Commit-Position: refs/heads/master@{#441030} |
