Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14805004:
InstantExtended: Prevent spurious themechanged/mostvisitedchanged events (Closed)
|
Created:
7 years, 7 months ago by Anuj Modified:
7 years, 7 months ago CC:
chromium-reviews, melevin, dhollowa+watch_chromium.org, dougw+watch_chromium.org, sreeram, gideonwald, dominich, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered, dcblack Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionInstantExtended: Prevent spurious themechanged/mostvisitedchanged events
Detect whether the ThemeBackgroundInfo and MostVisitedItems have really
changed.
R=samarth@chromium.org
BUG=225760
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198512
Patch Set 1 : Prevent event triggers from searchbox #
Total comments: 4
Patch Set 2 : Use Did<item>Change methods #
Total comments: 3
Patch Set 3 : Use namespace methods #
Total comments: 2
Patch Set 4 : Use Equal check, not changed #
Total comments: 6
Patch Set 5 : Addressed last set of comments #
Total comments: 6
Patch Set 6 : Typos again #Messages
Total messages: 20 (0 generated)
This is not right. There are many ways you could open an NTP in the background. Then if you switch to it from another NTP, it wouldn't have any theme/MV data. Conversely (and more importantly), if you switch to an NTP from a non-NTP tab, SearchModeChanged will be fired, which means you'll resend the data, so the flicker will still be there. Instead, you should suppress the events by making SearchBox look at the current & new values of the data to see if it has in fact changed, as mentioned in comment #5 on the bug.
On 2013/05/03 06:26:11, sreeram wrote: > This is not right. There are many ways you could open an NTP in the background. > Then if you switch to it from another NTP, it wouldn't have any theme/MV data. > Conversely (and more importantly), if you switch to an NTP from a non-NTP tab, > SearchModeChanged will be fired, which means you'll resend the data, so the > flicker will still be there. > > Instead, you should suppress the events by making SearchBox look at the current > & new values of the data to see if it has in fact changed, as mentioned in > comment #5 on the bug. What Sreeram said. Take a look at SearchBox::OnMostVisitedChanged in chrome/renderer/searchbox/searchbox.cc. We should do the check here. (Note how there are similar checks in that file, e.g. in OnKeyCaptureChange.) Thanks, Samarth
Just tried out your patch and it works great! Thanks, Samarth https://codereview.chromium.org/14805004/diff/7001/chrome/common/instant_types.h File chrome/common/instant_types.h (right): https://codereview.chromium.org/14805004/diff/7001/chrome/common/instant_type... chrome/common/instant_types.h:160: bool operator==(const ThemeBackgroundInfo& other) const { Don't overload operators please. Just add a helper function in searchbox.cc. https://codereview.chromium.org/14805004/diff/7001/chrome/renderer/searchbox/... File chrome/renderer/searchbox/searchbox.cc (right): https://codereview.chromium.org/14805004/diff/7001/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox.cc:335: if (theme_info == theme_info_) { nit: no braces for 1-line conditionals in Chrome. https://codereview.chromium.org/14805004/diff/7001/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox.cc:382: bool most_visited_changed = oldItems.size() != items.size(); if (oldsize != newsize) return; https://codereview.chromium.org/14805004/diff/7001/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox.cc:388: most_visited_changed = true; Just return here.
Actually to be consistent, I'd just add helper did-change? functions for both theme and most visited.
On 2013/05/03 21:25:55, samarth wrote: > Actually to be consistent, I'd just add helper did-change? functions for both > theme and most visited. Updated - ptal.
https://codereview.chromium.org/14805004/diff/13001/chrome/renderer/searchbox... File chrome/renderer/searchbox/searchbox.cc (right): https://codereview.chromium.org/14805004/diff/13001/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox.cc:334: bool SearchBox::DidThemeInfoChange(const ThemeBackgroundInfo& theme_info) { nit: Please move the anonymous namespace at the top of the file and just pass in the old theme info. https://codereview.chromium.org/14805004/diff/13001/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox.cc:346: theme_info_.has_attribution != theme_info.has_attribution; Please also add a comment in instant_types.h to keep the fields in sync here if new fields are added. https://codereview.chromium.org/14805004/diff/13001/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox.cc:391: bool SearchBox::DidMostVisitedItemsChange( Likewise (anonymous namespace please).
lgtm https://codereview.chromium.org/14805004/diff/19001/chrome/renderer/searchbox... File chrome/renderer/searchbox/searchbox.cc (right): https://codereview.chromium.org/14805004/diff/19001/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox.cc:43: bool DidMostVisitedItemsChange( Sorry for the back and forth, but one more change: please make this (and the other one) the reverse, i.e., AreMostVisitedItemsEqual() and flip the checks below. This way, if someone does forget to update the fields here, this will continue to be functionally correct -- we'll send some unnecessary updates but never miss an actual update. Right now, it's the opposite.
https://codereview.chromium.org/14805004/diff/19001/chrome/renderer/searchbox... File chrome/renderer/searchbox/searchbox.cc (right): https://codereview.chromium.org/14805004/diff/19001/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox.cc:43: bool DidMostVisitedItemsChange( On 2013/05/04 00:41:54, samarth wrote: > Sorry for the back and forth, but one more change: please make this (and the > other one) the reverse, i.e., AreMostVisitedItemsEqual() and flip the checks > below. > > This way, if someone does forget to update the fields here, this will continue > to be functionally correct -- we'll send some unnecessary updates but never miss > an actual update. Right now, it's the opposite. Done.
slgtm Thanks! Samarth
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/14805004/23001
A couple of nits below. https://chromiumcodereview.appspot.com/14805004/diff/23001/chrome/renderer/se... File chrome/renderer/searchbox/searchbox.cc (right): https://chromiumcodereview.appspot.com/14805004/diff/23001/chrome/renderer/se... chrome/renderer/searchbox/searchbox.cc:50: InstantMostVisitedItem new_item = items[i].second; "const InstantMostVisitedItem&" in both lines above. https://chromiumcodereview.appspot.com/14805004/diff/23001/chrome/renderer/se... chrome/renderer/searchbox/searchbox.cc:50: InstantMostVisitedItem new_item = items[i].second; It's mildly inconsistent to have "items" and "old_items", but "new_item" and "old_item". I'd rather that you just made all the non-old stuff above "new_*" (i.e., "new_theme_info" and "new_items"). https://chromiumcodereview.appspot.com/14805004/diff/23001/chrome/renderer/se... chrome/renderer/searchbox/searchbox.cc:52: return false; Nit: No braces.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/14805004/23001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/14805004/42001
https://chromiumcodereview.appspot.com/14805004/diff/42001/chrome/renderer/se... File chrome/renderer/searchbox/searchbox.cc (right): https://chromiumcodereview.appspot.com/14805004/diff/42001/chrome/renderer/se... chrome/renderer/searchbox/searchbox.cc:44: const std::vector<InstantMostVisitedItemIDPair>& new_tems, Typo: new_tems -> new_items https://chromiumcodereview.appspot.com/14805004/diff/42001/chrome/renderer/se... chrome/renderer/searchbox/searchbox.cc:49: InstantMostVisitedItem old_item = old_items[i].second; const InstantMostVisitedItem& https://chromiumcodereview.appspot.com/14805004/diff/42001/chrome/renderer/se... chrome/renderer/searchbox/searchbox.cc:50: InstantMostVisitedItem new_item = new_tems[i].second; const InstantMostVisitedItem&
https://chromiumcodereview.appspot.com/14805004/diff/23001/chrome/renderer/se... File chrome/renderer/searchbox/searchbox.cc (right): https://chromiumcodereview.appspot.com/14805004/diff/23001/chrome/renderer/se... chrome/renderer/searchbox/searchbox.cc:50: InstantMostVisitedItem new_item = items[i].second; On 2013/05/05 02:19:12, sreeram wrote: > "const InstantMostVisitedItem&" in both lines above. Done. https://chromiumcodereview.appspot.com/14805004/diff/23001/chrome/renderer/se... chrome/renderer/searchbox/searchbox.cc:50: InstantMostVisitedItem new_item = items[i].second; On 2013/05/05 02:19:12, sreeram wrote: > It's mildly inconsistent to have "items" and "old_items", but "new_item" and > "old_item". > > I'd rather that you just made all the non-old stuff above "new_*" (i.e., > "new_theme_info" and "new_items"). Done. https://chromiumcodereview.appspot.com/14805004/diff/23001/chrome/renderer/se... chrome/renderer/searchbox/searchbox.cc:52: return false; On 2013/05/05 02:19:12, sreeram wrote: > Nit: No braces. Done. https://chromiumcodereview.appspot.com/14805004/diff/42001/chrome/renderer/se... File chrome/renderer/searchbox/searchbox.cc (right): https://chromiumcodereview.appspot.com/14805004/diff/42001/chrome/renderer/se... chrome/renderer/searchbox/searchbox.cc:44: const std::vector<InstantMostVisitedItemIDPair>& new_tems, On 2013/05/06 14:31:56, sreeram wrote: > Typo: new_tems -> new_items Done. https://chromiumcodereview.appspot.com/14805004/diff/42001/chrome/renderer/se... chrome/renderer/searchbox/searchbox.cc:49: InstantMostVisitedItem old_item = old_items[i].second; On 2013/05/06 14:31:56, sreeram wrote: > const InstantMostVisitedItem& Done. https://chromiumcodereview.appspot.com/14805004/diff/42001/chrome/renderer/se... chrome/renderer/searchbox/searchbox.cc:50: InstantMostVisitedItem new_item = new_tems[i].second; On 2013/05/06 14:31:56, sreeram wrote: > const InstantMostVisitedItem& Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/14805004/56001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/14805004/56001
lgtm
Message was sent while issue was closed.
Change committed as 198512 |
