Chromium Code Reviews Chromium Code Reviews
Issue
1142153002:
Simplify infobar expiry. (Closed)
DescriptionSimplify infobar expiry.
Some of the changes below may have a functional effect, but I don't _think_ they will, if they do it should be minor, and I don't know another way of finding out than trying to make this change and seeing what happens. (Besides simplification, this change is also intended to unblock https://codereview.chromium.org/1139823006/ , which doesn't want to affect infobar behavior but without this change might do so.)
* Moved the check for "!details.did_replace_entry" from ConfirmInfoBarDelegate up to the base class. Very few people subclass the base class anyway, and for those who do, there's no obvious reason not to do this. I actually suggested doing this in a review several years ago but it never happened.
* Removed InfoBarDelegate::ShouldExpireInternal(), which didn't seem to be doing much useful. I think the unique ID check was basically doing the same thing as the "is_navigation_to_different_page" bit that was already checked in ShouldExpire() (which didn't exist when the unique IDs were added long ago). So if we got to ShouldExpireInternal() at all, that would be true regardless, and thus that helper would always return true. (Incidentally, reloads _are_ treated as "navigations to a different page", so we still dismiss infobars by default on reload. If this wasn't true we'd have already been broken anyway, since ShouldExpire() would have early-returned false.)
* Removed ProtectedMediaIdentifierInfoBarDelegate::ShouldExpireInternal(). I discussed this with kkimlabs@, the original author, and neither of us can understand why it was necessary to begin with. We both think this should be removed.
* Removed the wonky TranslateInfoBarDelegate::ShouldExpire(). Long ago, this used to check a bit called "is_auto", because it was trying to allow the infobar to be dismissed even for automatic navigations, whereas other infobars wanted to only be dismissed for user-driven ones. However, the "is_auto" bit was later removed, and the replacement code didn't really work the same way, and besides this, the conditional didn't really make sense, since "!details.is_main_frame" implies "!details.is_navigation_to_different_page". So we basically had a function that would call InfoBarDelegate::ShouldExpireInternal() for all main-frame navigations. But since, as I said above, I think the unique ID check there was functionally the same as checking for a "navigation to a different page"... I think all this ended up just doing the same thing as the original base function. And I couldn't check because the translate infobar no longer exists for views anyway. So whatever.
* Removed copy-and-pasted code that wasn't needed.
BUG=none
TEST=none
Committed: https://crrev.com/34cc3c8d2b014a6413709ba88249eb2bd3c933a7
Cr-Commit-Position: refs/heads/master@{#330817}
Patch Set 1 #Patch Set 2 : Fix Android compile #
Total comments: 2
Messages
Total messages: 16 (4 generated)
|
