Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1360873002:
[Contextual Search] Prevents Panel from becoming blank if opened before resolving. (Closed)
|
Created:
5 years, 3 months ago by pedro (no code reviews) Modified:
5 years, 2 months ago Reviewers:
Donn Denman CC:
chromium-reviews, mdjones Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Contextual Search] Prevents Panel from becoming blank if opened before resolving.
This was a regression caused by:
https://codereview.chromium.org/1304013002/
BUG=534519
Committed: https://crrev.com/309d5989e68f7a8cab44ed2ba0fff843eb887d46
Cr-Commit-Position: refs/heads/master@{#350610}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Sync & rebase #Patch Set 3 : Fixing broken testswq #
Messages
Total messages: 23 (10 generated)
pedrosimonetti@chromium.org changed reviewers: + donnd@chromium.org
Donn, please take a look at this change. Matt, this is just a FYI so you are aware of the changes (which will need to be merged in your upcoming changes).
https://codereview.chromium.org/1360873002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java (right): https://codereview.chromium.org/1360873002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:552: getManagementDelegate().onContentViewDestroyed(); It looks to me like keeping this line outside the conditional would be fine, and that makes more sense to me. Is there a particular reason why you moved it too?
https://codereview.chromium.org/1360873002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java (right): https://codereview.chromium.org/1360873002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:552: getManagementDelegate().onContentViewDestroyed(); On 2015/09/22 20:42:18, Donn Denman wrote: > It looks to me like keeping this line outside the conditional would be fine, and > that makes more sense to me. Is there a particular reason why you moved it too? Yes, there is. destroyContentView() might get called multiple times, and I felt this method wasn't doing the right thing. If the ContentViewCore is already destroyed we shouldn't fire the onContentViewDestroyed() twice.
lgtm https://codereview.chromium.org/1360873002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java (right): https://codereview.chromium.org/1360873002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:552: getManagementDelegate().onContentViewDestroyed(); On 2015/09/22 20:51:31, pedrosimonetti wrote: > On 2015/09/22 20:42:18, Donn Denman wrote: > > It looks to me like keeping this line outside the conditional would be fine, > and > > that makes more sense to me. Is there a particular reason why you moved it > too? > > Yes, there is. destroyContentView() might get called multiple times, and I felt > this method wasn't doing the right thing. If the ContentViewCore is already > destroyed we shouldn't fire the onContentViewDestroyed() twice. Acknowledged.
The CQ bit was checked by pedrosimonetti@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1360873002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1360873002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by pedrosimonetti@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1360873002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1360873002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by pedrosimonetti@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from donnd@chromium.org Link to the patchset: https://codereview.chromium.org/1360873002/#ps20001 (title: "Sync & rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1360873002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1360873002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by pedrosimonetti@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from donnd@chromium.org Link to the patchset: https://codereview.chromium.org/1360873002/#ps40001 (title: "Fixing broken testswq")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1360873002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1360873002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/309d5989e68f7a8cab44ed2ba0fff843eb887d46 Cr-Commit-Position: refs/heads/master@{#350610} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
