299155 - Force-reload should invalidate the page's associated favicon.

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Wevah (Nate Weaver)]

The summary pretty much says it all. Currently, if the favicon changes after the
page has been visited, only a cache wipe will make Camino grab the new one.

Comment 1

[Samuel Sidler (old account; do not CC)]

It'd be good, but probably not for 1.0.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

STR:

1) Visit a page with a favicon (on some server you control)
2) Change the favicon to something else (or add a png using link rel to supplement a favicon.ico or whatever)
3) Visit the page again; note the favicon displayed is still favicon#1
4) Shift-reload; note the favicon displayed is still favicon#1

Expected: Step 4 should end with favicon#2 being displayed

Comment 3

[Chris Lawson (gone)]

Simon, a hint, please? :)

cl

Comment 4

[Simon Fraser [no longer active]]

You'll have to either purge from the caches the site icon and associated data for the current URL when you detect a force-reload, or pass the force-reload flags down into site icon loading.

Comment 5

[froodian (Ian Leue)]

Attachment: Patch

A couple things here:

1. The portion in PreferenceManager.h is just a warning fix (SiteIconCache calls the method).
2. It feels kind of sketchy to me to be doing this as low down as CHBrowserView, but if I did it in BWC I'd have to triple some code or do some reload logic re-factoring.  Hmm... maybe that should be done anyway...

Comment 6

[Chris Lawson (gone)]

Comment on attachment 247511 [details] [diff] [review]
Patch

Index: src/preferences/PreferenceManager.h
===================================================================

@@ -79,16 +79,17 @@
 - (void)setPref:(const char*)prefName toString:(NSString*)value;
 - (void)setPref:(const char*)prefName toInt:(int)value;
 - (void)setPref:(const char*)prefName toBoolean:(BOOL)value;
 
 - (void)clearPref:(const char*)prefName;
 
 // the path to the user profile's root folder, used by camino 0.8+
 - (NSString*)profilePath;
+- (NSString*)cacheParentDirPath;

It's not at all clear to me why this is necessary. Rest of it looks good, r=me with a satisfactory explanation of why that's needed (or with it removed if it's not).

Comment 7

[Chris Lawson (gone)]

(In reply to comment #6)
> It's not at all clear to me why this is necessary.

I am an idiot, and read the patch without seeing comment 5. Please ignore me. :)

On a related note, I can't think of any compelling reason why BWC should know about a page's favicon any more than CHBrowserView would.

Comment 8

[Stuart Morgan]

(In reply to comment #7)
> On a related note, I can't think of any compelling reason why BWC should know
> about a page's favicon any more than CHBrowserView would.

Because favicons are used by BWC, and not by CHBrowserView. This seems like it should be done higher up.

Comment 9

[froodian (Ian Leue)]

Attachment: Patch

Does it higher up.  As I mentioned in comment 5, this included some reload logic refactoring (some of which should have been done long ago) in order to do it gracefully.

Comment 10

[Stuart Morgan]

Comment on attachment 248674 [details] [diff] [review]
Patch

Sorry this sat for so long :P

>+// uuid arg is optional (for speed)
>+- (void)removeImageForURL:(NSString*)inURL uuid:(NSString*)inUUID;

I haven't looked too much into this, but I assume that uuid is meaningless outside this class? If so, rather than making this public, make a public removeImageForURL that calls it with a nil UUID.

>+- (unsigned int)reloadFlagsForSender:(id)sender;

Is there an advantage to having this method rather than just passing the sender into the core reload method? It looks like it could just be folded in.

> // run a modal according to the users pref on opening a feed
> - (BOOL)shouldWarnBeforeOpeningFeed;
> - (void)buildFeedsDetectedListMenu:(NSNotification*)notifer; 
> - (IBAction)openFeedInExternalApp:(id)sender;
> 
> - (void)insertForceAlternatesIntoMenu:(NSMenu *)inMenu;
> - (BOOL)prepareSpellingSuggestionMenu:(NSMenu*)inMenu tag:(int)inTag;
> 
>@@ -2985,28 +2990,61 @@
> 
> - (IBAction)forward:(id)aSender
> {
>   [[mBrowserView getBrowserView] goForward];
> }
> 
> - (IBAction)reload:(id)aSender
> {
>-  unsigned int reloadFlags = NSLoadFlagsNone;
>+  [self reloadBrowserWrapper:mBrowserView withFlags:[self reloadFlagsForSender:aSender]];
>+}
>+
>+- (IBAction)reloadSendersTab:(id)sender
>+{
>+  if ([sender isMemberOfClass:[NSMenuItem class]]) {
>+    BrowserTabViewItem* tabViewItem = [mTabBrowser itemWithTag:[sender tag]];
>+    if (tabViewItem)
>+      [self reloadBrowserWrapper:[tabViewItem view] withFlags:[self reloadFlagsForSender:sender]];
>+  }
>+}
> 
>-  if ([aSender respondsToSelector:@selector(keyEquivalent)]) {
>+- (IBAction)reloadAllTabs:(id)sender
>+{
>+  NSEnumerator* tabsEnum = [[mTabBrowser tabViewItems] objectEnumerator];
>+  BrowserTabViewItem* curTabItem;
>+  while ((curTabItem = [tabsEnum nextObject])) {
>+    if ([curTabItem isKindOfClass:[BrowserTabViewItem class]])
>+      [self reloadBrowserWrapper:[curTabItem view] withFlags:[self reloadFlagsForSender:sender]];
>+  }
>+}
>+
>+- (void)reloadBrowserWrapper:(BrowserWrapper *)inWrapper withFlags:(unsigned int)reloadFlags
>+{
>+  if (reloadFlags == NSLoadFlagsBypassCacheAndProxy) {
>+    // Toss the favicon
>+    NSString* faviconURL = [[SiteIconProvider sharedFavoriteIconProvider] favoriteIconURLFromPageURL:[inWrapper getCurrentURI]];
>+    [[SiteIconCache sharedSiteIconCache] removeImageForURL:faviconURL uuid:nil];
>+  }
>+
>+  [[inWrapper getBrowserView] reload:reloadFlags];
>+}
>+
>+- (unsigned int)reloadFlagsForSender:(id)sender
>+{
>+  if ([sender respondsToSelector:@selector(keyEquivalent)]) {
>     // Capital R tests for shift when there's a keyEquivalent, keyEquivalentModifierMask tests when there isn't
>-    if ([[aSender keyEquivalent] isEqualToString:@"R"] || ([aSender keyEquivalentModifierMask] & NSShiftKeyMask))
>-      reloadFlags = NSLoadFlagsBypassCacheAndProxy;
>+    if ([[sender keyEquivalent] isEqualToString:@"R"] || ([sender keyEquivalentModifierMask] & NSShiftKeyMask))
>+      return NSLoadFlagsBypassCacheAndProxy;
>   }
>   // It's a toolbar button, so we test for shift using modifierFlags
>   else if ([[NSApp currentEvent] modifierFlags] & NSShiftKeyMask)
>-    reloadFlags = NSLoadFlagsBypassCacheAndProxy;
>+    return NSLoadFlagsBypassCacheAndProxy;
> 
>-  [[mBrowserView getBrowserView] reload:reloadFlags];
>+  return NSLoadFlagsNone;
> }
> 
> - (IBAction)stop:(id)aSender
> {
>   [[mBrowserView getBrowserView] stop:NSStopLoadAll];
> }
> 
> - (IBAction)home:(id)aSender
>@@ -3397,52 +3435,16 @@
>         
>         [[doomedItem view] windowClosed];
>         [mTabBrowser removeTabViewItem:doomedItem];
>       }
>     }
>   }
> }
> 
>-- (IBAction)reloadSendersTab:(id)sender
>-{
>-  if ([sender isMemberOfClass:[NSMenuItem class]]) {
>-    BrowserTabViewItem* tabViewItem = [mTabBrowser itemWithTag:[sender tag]];
>-    if (tabViewItem) {
>-      unsigned int reloadFlags = NSLoadFlagsNone;
>-      // Capital R tests for shift when there's a keyEquivalent, keyEquivalentModifierMask tests when there isn't
>-      if ([[sender keyEquivalent] isEqualToString:@"R"] || ([sender keyEquivalentModifierMask] & NSShiftKeyMask))
>-        reloadFlags = NSLoadFlagsBypassCacheAndProxy;
>-
>-      [[[tabViewItem view] getBrowserView] reload:reloadFlags];
>-    }
>-  }
>-}
>-
>-- (IBAction)reloadAllTabs:(id)sender
>-{
>-  unsigned int reloadFlags = NSLoadFlagsNone;
>-
>-  if ([sender respondsToSelector:@selector(keyEquivalent)]) {
>-    // Capital R tests for shift when there's a keyEquivalent, keyEquivalentModifierMask tests when there isn't
>-    if ([[sender keyEquivalent] isEqualToString:@"R"] || ([sender keyEquivalentModifierMask] & NSShiftKeyMask))
>-      reloadFlags = NSLoadFlagsBypassCacheAndProxy;
>-  }
>-  // It's a toolbar button, so we test for shift using modifierFlags
>-  else if ([[NSApp currentEvent] modifierFlags] & NSShiftKeyMask)
>-    reloadFlags = NSLoadFlagsBypassCacheAndProxy;
>-
>-  NSEnumerator* tabsEnum = [[mTabBrowser tabViewItems] objectEnumerator];
>-  BrowserTabViewItem* curTabItem;
>-  while ((curTabItem = [tabsEnum nextObject])) {
>-    if ([curTabItem isKindOfClass:[BrowserTabViewItem class]])
>-      [[[curTabItem view] getBrowserView] reload:reloadFlags];
>-  }
>-}
>-
> - (IBAction)moveTabToNewWindow:(id)sender
> {
>   if ([sender isMemberOfClass:[NSMenuItem class]])
>   {
>     BrowserTabViewItem* tabViewItem = [mTabBrowser itemWithTag:[sender tag]];
>     if (tabViewItem)
>     {
>       NSString* url = [[tabViewItem view] getCurrentURI];
>Index: src/preferences/PreferenceManager.h
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/preferences/PreferenceManager.h,v
>retrieving revision 1.21
>diff -u -8 -r1.21 PreferenceManager.h
>--- src/preferences/PreferenceManager.h	15 Nov 2006 23:39:15 -0000	1.21
>+++ src/preferences/PreferenceManager.h	14 Dec 2006 18:35:41 -0000
>@@ -79,16 +79,17 @@
> - (void)setPref:(const char*)prefName toString:(NSString*)value;
> - (void)setPref:(const char*)prefName toInt:(int)value;
> - (void)setPref:(const char*)prefName toBoolean:(BOOL)value;
> 
> - (void)clearPref:(const char*)prefName;
> 
> // the path to the user profile's root folder, used by camino 0.8+
> - (NSString*)profilePath;
>+- (NSString*)cacheParentDirPath;
> 
> // turn notifications on and off when the given pref changes. 
> // if not nil, inObject is used at the 'object' of the resulting notification.
> - (void)addObserver:(id)inObject forPref:(const char*)inPrefName;
> - (void)removeObserver:(id)inObject;    // remove from all prefs that it observes
> - (void)removeObserver:(id)inObject forPref:(const char*)inPrefName;
> 
> @end

Comment 11

[froodian (Ian Leue)]

Attachment: Patch

(In reply to comment #10)
> (From update of attachment 248674 [details] [diff] [review])
> Sorry this sat for so long :P
> 
> >+// uuid arg is optional (for speed)
> >+- (void)removeImageForURL:(NSString*)inURL uuid:(NSString*)inUUID;
> 
> I haven't looked too much into this, but I assume that uuid is meaningless
> outside this class? If so, rather than making this public, make a public
> removeImageForURL that calls it with a nil UUID.

Done

> 
> >+- (unsigned int)reloadFlagsForSender:(id)sender;
> 
> Is there an advantage to having this method rather than just passing the sender
> into the core reload method? It looks like it could just be folded in.

No reason really.  I had considered doing it the way you suggest too - this patch does that.

Comment 12

[Stuart Morgan]

Comment on attachment 251599 [details] [diff] [review]
Patch

Sorry, two more things I didn't catch:

>+#import "SiteIconCache.h"

I think SiteIconCache should continue to be private to SiteIconProvider; no-one else should need to know how SIP does its work. So there needs to be a purge method in SIP that passes through to SIC.

>+#import "SiteIconProvider.h"

This one I feel stupid about, since I said to push it to BWC... I thought BWC already dealt with the site icon provider, but it doesn't, BrowserWrapper does. Since that's the layer those things happen at (and since if there's one thing BWC absolutely doesn't need it's to be tied to even more pieces), I think there should be a reload method added to BW that is called instead of a direct call past BW from BWC, and BW's reload should do this.

Please do keep the much-needed BWC reload refactoring though!

Comment 13

[froodian (Ian Leue)]

Attachment: Patch

Done

Comment 14

[Stuart Morgan]

Comment on attachment 252067 [details] [diff] [review]
Patch

What's here looks good, but the diff looks like it's missing the SiteIconProvider changes.

Comment 15

[froodian (Ian Leue)]

Attachment: Patch

Whoops.  Fixed.

Comment 16

[Stuart Morgan]

Comment on attachment 252097 [details] [diff] [review]
Patch

Looks great; r=me

Comment 17

[Mike Pinkerton (not reading bugmail)]

can we get some function-level comments on all the new methods added?

Comment 18

[froodian (Ian Leue)]

Attachment: Patch

Better documented.

Some things to note:
- The portion in BWC is largely refactoring, and moving the similar "reload" methods to be next to the plain-vanilla "reload"
- The PreferenceMaager portion is just a warning fix for an existing call

Comment 19

[froodian (Ian Leue)]

Checked in on trunk and MOZILLA_1_8_BRANCH