Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2678053002:
Convert ContentTranslateDriver to use the new navigation callbacks. (Closed)
|
Created:
3 years, 10 months ago by jam Modified:
3 years, 10 months ago Reviewers:
groby-ooo-7-16 CC:
chromium-reviews, jam, darin-cc_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert ContentTranslateDriver to use the new navigation callbacks.
BUG=682002
Review-Url: https://codereview.chromium.org/2678053002
Cr-Commit-Position: refs/heads/master@{#448678}
Committed: https://chromium.googlesource.com/chromium/src/+/18d88253e229f941b6bd2a7a10f9961e7c5be05c
Patch Set 1 #
Total comments: 7
Messages
Total messages: 21 (15 generated)
The CQ bit was checked by jam@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...
Patchset #1 (id:1) has been deleted
Description was changed from ========== Convert ContentTranslateDriver to use the new navigation callbacks. BUG=682002 ========== to ========== Convert ContentTranslateDriver to use the new navigation callbacks. BUG=682002 ==========
jam@chromium.org changed reviewers: + groby@chromium.org
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jam@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...
Looks OK to me, I'm just not 100% sure on a few questions. LGTM, since I assume you know navigation better than I do, but would love to have the answers :) https://codereview.chromium.org/2678053002/diff/20001/components/translate/co... File components/translate/content/browser/content_translate_driver.cc (right): https://codereview.chromium.org/2678053002/diff/20001/components/translate/co... components/translate/content/browser/content_translate_driver.cc:217: if (!navigation_handle->HasCommitted()) Why do we need to check HasCommitted()? Isn't that implied in DidFinish? https://codereview.chromium.org/2678053002/diff/20001/components/translate/co... components/translate/content/browser/content_translate_driver.cc:222: navigation_handle->GetReloadType() != content::ReloadType::NONE || I assume ReloadType()!=NONE is the equivalent of PageTransitionCoreTypeIs()==PAGE_TRANSITION_RELOAD? (Sidebar: I sure wish NavigationHandle did just have an IsReload() member...) https://codereview.chromium.org/2678053002/diff/20001/components/translate/co... components/translate/content/browser/content_translate_driver.cc:225: navigation_handle->IsSamePage(), navigation_handle->IsInMainFrame(), Should this pass through the |navigation_handle| instead?
https://codereview.chromium.org/2678053002/diff/20001/components/translate/co... File components/translate/content/browser/content_translate_driver.cc (right): https://codereview.chromium.org/2678053002/diff/20001/components/translate/co... components/translate/content/browser/content_translate_driver.cc:217: if (!navigation_handle->HasCommitted()) On 2017/02/07 18:31:08, groby wrote: > Why do we need to check HasCommitted()? Isn't that implied in DidFinish? If the response is a download or 204/205, then the navigation finishes but doesn't commit (i.e. the previous page is the one that's in the tab) https://codereview.chromium.org/2678053002/diff/20001/components/translate/co... components/translate/content/browser/content_translate_driver.cc:222: navigation_handle->GetReloadType() != content::ReloadType::NONE || On 2017/02/07 18:31:08, groby wrote: > I assume ReloadType()!=NONE is the equivalent of > PageTransitionCoreTypeIs()==PAGE_TRANSITION_RELOAD? Correct > > (Sidebar: I sure wish NavigationHandle did just have an IsReload() member...) I hear you (I had to do this in a couple of other places as well and I did find it a little awkward). The tradeoff is that the class is already pretty large, so adding redundant helpers could be a slippery slope. https://codereview.chromium.org/2678053002/diff/20001/components/translate/co... components/translate/content/browser/content_translate_driver.cc:225: navigation_handle->IsSamePage(), navigation_handle->IsInMainFrame(), On 2017/02/07 18:31:08, groby wrote: > Should this pass through the |navigation_handle| instead? That's calling out to shared code with ios, so it doesn't use any content types.
The CQ bit was unchecked by jam@chromium.org
The CQ bit was checked by jam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2678053002/diff/20001/components/translate/co... File components/translate/content/browser/content_translate_driver.cc (right): https://codereview.chromium.org/2678053002/diff/20001/components/translate/co... components/translate/content/browser/content_translate_driver.cc:217: if (!navigation_handle->HasCommitted()) On 2017/02/07 18:35:17, jam wrote: > On 2017/02/07 18:31:08, groby wrote: > > Why do we need to check HasCommitted()? Isn't that implied in DidFinish? > > If the response is a download or 204/205, then the navigation finishes but > doesn't commit (i.e. the previous page is the one that's in the tab) btw thanks for reminding me, I wanted to add some comments to NH to write this out. I'll do that in a followup
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486492526346110,
"parent_rev": "2ff49b42058b3e5604498f6999b85f16bfc01116", "commit_rev":
"18d88253e229f941b6bd2a7a10f9961e7c5be05c"}
Message was sent while issue was closed.
Description was changed from ========== Convert ContentTranslateDriver to use the new navigation callbacks. BUG=682002 ========== to ========== Convert ContentTranslateDriver to use the new navigation callbacks. BUG=682002 Review-Url: https://codereview.chromium.org/2678053002 Cr-Commit-Position: refs/heads/master@{#448678} Committed: https://chromium.googlesource.com/chromium/src/+/18d88253e229f941b6bd2a7a10f9... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:20001) as https://chromium.googlesource.com/chromium/src/+/18d88253e229f941b6bd2a7a10f9... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
