Bug 17526 – REGRESSION: iframes are added to Safari's History menu

• Component: History
• Status: RESOLVED FIXED

When you open http://cnn.com/ in TOT, Safari's History menu is cluttered with all the iframes on the page. I think this is a result of <http://trac.webkit.org/projects/webkit/changeset/30549> (bug 16770).

[Darin Adler]

I guess we need a separate concept of "in the history" for visited links than the one for the history in the application. I would like to talk this one over with John Sullivan.

[Darin Adler]

Created attachment 19383 [details]
work in progress

[Darin Adler]

Created attachment 19477 [details]
work in progress

*** Bug 17641 has been marked as a duplicate of this bug. ***

[Darin Adler]

Created attachment 19539 [details]
patch

[Darin Adler]

Created attachment 19542 [details]
patch

Comment on attachment 19542 [details]
patch

Review of the WebCore part: mostly nit-picking, but also a couple of serious problems.

-        Vector<CSSMutableStyleDeclaration*> m_additionalAttributeStyleDecls;
-        
+        Vector<CSSMutableStyleDeclaration*> m_additionalAttributeStyleDecls;        

You accidentally added trailing spaces.

+        if (otherPage != page)
+            for (Frame* frame = otherPage->mainFrame(); frame; frame = frame->tree()->traverseNext())
+                if (frame->tree()->name() == name)
+                    return frame;

The outer if and for loop above need braces.

+    HashSet<Page*>::iterator end = allPages->end();
+    for (HashSet<Page*>::iterator it = allPages->begin(); it != end; ++it) {
+        if (PageGroup* group = (*it)->groupPtr())
+            groups.add(group);
+    }
+    HashSet<PageGroup*>::iterator e2 = groups.end();

"e2" without an "e1" is weird. How about "pagesEnd" and "groupsEnd" for the two local variables?

+    class KURL;

Any reason why this one is out of alphabetical order?

+        PageGroup();

Is this constructor needed?

+    if (length < 3)
+        return false;
+    unsigned loopLimit = length - 2;
+    for (unsigned i = 0; i < loopLimit; ++i)
+        if (characters[i] == ':' && characters[i + 1] == '/' && characters[i + 2] == '/')
+            return true;

The for loop needs braces. Also, is there a reason why this function has an explicit early return, whereas the others just set loopLimit to 0?

+    if (!matchLetter(characters[0], 'h')
+            || matchLetter(characters[1], 't')
+            || matchLetter(characters[2], 't')
+            || matchLetter(characters[3], 'p'))
+        return false;

I don't understand this condition. Looks like this will return false for "http".

I also wonder if URLs terminating with a slash are common enough to check for that first.

How is the {defer, resume}VisitedLinkUpdates() mechanism supposed to work?

+    unsigned pathEnd() const { return pathEndPos; }
+    unsigned pathAfterLastSlash() const { return pathEndPos; }

This looks wrong. Looking at KURL I don't see that it caches the offset after the last slash in the path, so you will need to add that.

Comment on attachment 19542 [details]
patch

[See comment #7 for a review of the WebCore part]

+        (-[WebHistory _addVisitedLinksToPageGroup:WebCore::]): Added. For use only inside WebKit.

prepare-ChangeLog messed up the method signature.

-#import "WebHistory.h"
-#import "WebHistoryPrivate.h"
+#import "WebHistoryInternal.h"
+#import "WebHistoryInternal.h"

You removed WebHistory.h and added WebHistoryInternal.h twice.

+    int itemLimit;
+    int ageInDaysLimit;

Should these be unsigned?

+- (BOOL)containsItemForURLString:(NSString *)URLString;
+- (BOOL)containsURL:(NSURL *)URL;
+- (WebHistoryItem *)itemForURL:(NSURL *)URL;
+- (WebHistoryItem *)itemForURLString:(NSString *)URLString;

It would be nice to change the order of the first two methods here.

+- (NSCalendarDate*)ageLimitDate;

Missing a space before the *.

+    if (count == 0)

Should be written as "if (count)".

+    NSMutableArray *discardedItems = [[NSMutableArray alloc] init];    

Doesn't this array leak in the "successful" branch?

r- because of the problems mentioned in comment #7 and the apparent leak.

[Darin Adler]

Thanks for the great review. Working on these things now.

(In reply to comment #7)
> +        Vector<CSSMutableStyleDeclaration*> m_additionalAttributeStyleDecls;   
> 
> You accidentally added trailing spaces.

Fixed.

> The outer if and for loop above need braces.

Fixed.

> How about "pagesEnd" and "groupsEnd" for the two local variables?

Great suggestion. Done.

> +    class KURL;
> 
> Any reason why this one is out of alphabetical order?

I know this class will soon be renamed to URL. I am conflicted about whether to sort it based on its current name or its future name.

> +        PageGroup();
> 
> Is this constructor needed?

Originally the PageGroup map stored actual PageGroup objects, not just pointers. With that design the empty constructor was needed. We no longer use that design so it's no longer needed. Also, I can now mark PageGroup as Noncopyable.

> +    if (length < 3)
> +        return false;
> +    unsigned loopLimit = length - 2;
> +    for (unsigned i = 0; i < loopLimit; ++i)
> +        if (characters[i] == ':' && characters[i + 1] == '/' && characters[i +
> 2] == '/')
> +            return true;
> 
> The for loop needs braces. Also, is there a reason why this function has an
> explicit early return, whereas the others just set loopLimit to 0?

No good reason. I changed all the functions to use early returns.

> +    if (!matchLetter(characters[0], 'h')
> +            || matchLetter(characters[1], 't')
> +            || matchLetter(characters[2], 't')
> +            || matchLetter(characters[3], 'p'))
> +        return false;
> 
> I don't understand this condition. Looks like this will return false for
> "http".

That was a bug. There were some missing ! operators, and I added them.

> I also wonder if URLs terminating with a slash are common enough to check for
> that first.

That's a good suggestion, worth exploring after I land. I'd prefer to leave the logic alone in this patch.

> How is the {defer, resume}VisitedLinkUpdates() mechanism supposed to work?

I don't really know exactly. It's sort of a placeholder for a future mechanism that updates visited links that are already displayed, but I haven't thought it through enough to be sure it will be helpful. I think there may need to be some sort of optimization to limit the complexity of the operation of adding lots of visited links all at once, but I haven't figured out the details; I'd probably keep a separate hash of "changed visited links" and then intersect that with the visited link sets of various views.

Let me know if you think I should remove this "scaffolding" from this patch.

> +    unsigned pathEnd() const { return pathEndPos; }
> +    unsigned pathAfterLastSlash() const { return pathEndPos; }
> 
> This looks wrong. Looking at KURL I don't see that it caches the offset after
> the last slash in the path, so you will need to add that.

You're absolutely right. Glad you caught this, although I must admit I knew about it.

I knew this was an outstanding task -- I shouldn't have marked the patch for review until it was resolved! I think I'll break out the KURL part and get it reviewed and landed first/separately.

(In reply to comment #9)

> > How is the {defer, resume}VisitedLinkUpdates() mechanism supposed to work?

[...]

> Let me know if you think I should remove this "scaffolding" from this patch.

I see it as a comment masquerading as code :-)

[Darin Adler]

(In reply to comment #8)
> +        (-[WebHistory _addVisitedLinksToPageGroup:WebCore::]): Added. For use
> only inside WebKit.
> 
> prepare-ChangeLog messed up the method signature.

Fixed.

> -#import "WebHistory.h"
> -#import "WebHistoryPrivate.h"
> +#import "WebHistoryInternal.h"
> +#import "WebHistoryInternal.h"
> 
> You removed WebHistory.h and added WebHistoryInternal.h twice.

Fixed. I meant to include only "WebHistoryInternal.h". The design here is that Internal headers always included the corresponding Private header first if any, and Private headers always include the corresponding Public header first if any. Then the implementation file includes the Internal header. That way all the headers get the "can be included standalone" testing and there are no redundant includes.

> +    int itemLimit;
> +    int ageInDaysLimit;
> 
> Should these be unsigned?

Maybe, but I'd prefer not to change that in this patch. I'm just moving this code around, trying to keep the changes to a minimum.

> +- (BOOL)containsItemForURLString:(NSString *)URLString;
> +- (BOOL)containsURL:(NSURL *)URL;
> +- (WebHistoryItem *)itemForURL:(NSURL *)URL;
> +- (WebHistoryItem *)itemForURLString:(NSString *)URLString;
> 
> It would be nice to change the order of the first two methods here.

Done. I hope we can remove some of these methods eventually.

> +- (NSCalendarDate*)ageLimitDate;
> 
> Missing a space before the *.

Fixed (in 3 places).

I *hate* our rule. I want a new rule for Objective-C vs. C++ stars! I'm hoping we can agree to a rule where we use the space in SPI and API headers, but everywhere else we use the same formatting for ObjC and C++.

> +    if (count == 0)
> 
> Should be written as "if (count)".

Done.

> +    NSMutableArray *discardedItems = [[NSMutableArray alloc] init];    
> 
> Doesn't this array leak in the "successful" branch?

It does! Fixed.

[Darin Adler]

(In reply to comment #10)
> > Let me know if you think I should remove this "scaffolding" from this patch.
> 
> I see it as a comment masquerading as code :-)

OK. Removed.

[Darin Adler]

Created attachment 19547 [details]
patch that addresses all of Mitz's comments except for the missing KURL logic

[Darin Adler]

Created attachment 19548 [details]
KURL work in progress

[Darin Adler]

Created attachment 19563 [details]
patch that adds new KURL features used by the main patch

[Darin Adler]

Comment on attachment 19563 [details]
patch that adds new KURL features used by the main patch

Sam reviewed this and we found and fixed one bug.

[Darin Adler]

Created attachment 19564 [details]
patch, with fixes requested by Mitz

[Darin Adler]

Comment on attachment 19563 [details]
patch that adds new KURL features used by the main patch

Landed this KURL patch.

Comment on attachment 19564 [details]
patch, with fixes requested by Mitz

r=me

[Darin Adler]

Committed revision 30840.

[Darin Adler]

Created attachment 19571 [details]
more work in progress