Summary: | REGRESSION: Certain iframes embedded in page listed in History menu | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Geoffrey F. Green <geoff-public> | ||||||||
Component: | History | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED WORKSFORME | ||||||||||
Severity: | Normal | CC: | mbritto, sroret | ||||||||
Priority: | P1 | Keywords: | Regression | ||||||||
Version: | 523.x (Safari 3) | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.4 | ||||||||||
URL: | http://wsj.com | ||||||||||
Attachments: |
|
Description
Geoffrey F. Green
2007-05-11 18:02:58 PDT
Created attachment 14499 [details]
Screenshot of History menu
Confirmed with a local debug build of WebKit r21427 with Safari 2.0.4 (419.3) on Mac OS X 10.4.9 (8P135). Giving this bug the full regression treatment. Created attachment 14998 [details]
Proposed patch
I added a condtion before calling the addHistoryForCurrentLocation() method from updateHistoryForInternalLoad : the frame must be the main frame to be added.
It works on the wall street journal website and all the layout tests are ok.
Created attachment 15051 [details]
cleaned up patch
Following the advices a reviewer gave me for another patch I submitted ; I cleaned up this one and added a brief description of the modifications.
Comment on attachment 15051 [details]
cleaned up patch
Fix looks good to me. Please add a layout test. I believe this could be tested using dumpBackForwardList().
The comment is subtly wrong:
+ // We only add the main frames to the history
We do, in fact, add loads of frames other than the main frame to the history, if they are not initial loads, but that would not apply here.
Comment on attachment 15051 [details]
cleaned up patch
Fix looks good to me. Please add a layout test. I believe this could be tested using dumpBackForwardList().
The comment is subtly wrong:
+ // We only add the main frames to the history
We do, in fact, add loads of frames other than the main frame to the history, if they are not initial loads, but that would not apply here.
Comment on attachment 15051 [details]
cleaned up patch
Fix looks good to me. Please add a layout test. I believe this could be tested using dumpBackForwardList().
The comment is subtly wrong:
+ // We only add the main frames to the history
We do, in fact, add loads of frames other than the main frame to the history, if they are not initial loads, but that would not apply here.
Comment on attachment 15051 [details]
cleaned up patch
Fix looks good to me. Please add a layout test. I believe this could be tested using dumpBackForwardList().
The comment is subtly wrong:
+ // We only add the main frames to the history
We do, in fact, add loads of frames other than the main frame to the history, if they are not initial loads, but that would not apply here.
I was trying to create a test layout when I noticed I can't reproduce the bug anymore (even without the fix I proposed...). I think my patch is no longer usefull :'( This sounds like it is no longer reproducible, i certainly can't repro with the steps given. Am marking as worksforme, Geoffrey if you can still reproduce this reopen the bug, maybe reproduction requires an account? Max are you sure that you were able to repro, and are no longer able to? I am away from a Mac, but I'll check on Monday as soon as I return. I was still seeing the error on a nightly as recently as last week, but I'll be able to be more precise in a couple of days. (In reply to comment #10) > Max are you sure that you were able to repro, and are no longer able to? > Yes I'm sure I was able to repro it (I've worked on it and I've proposed a patch about it). But now I can't repro it anymore. The only thing I changed since may be Safari (from 2.0.4 to 3.0) but I don't think it's related. Original reporter here. Unable to reproduce with r24399. It was intermittently reproducible as of a couple of nightlies ago, but before I had the chance to pin it down, it vanished completely. Looks like it's been fixed. I'll keep checking and will update if it recurs. |