Closed
Bug 455877
Opened 16 years ago
Closed 12 years ago
port bug 254714 to SeaMonkey (blank location bar while loading page in a new tab/window)
Categories
(SeaMonkey :: Location Bar, defect)
SeaMonkey
Location Bar
Tracking
(Not tracked)
RESOLVED
WORKSFORME
People
(Reporter: misak.bugzilla, Assigned: misak.bugzilla)
References
Details
Attachments
(1 file, 4 obsolete files)
2.88 KB,
patch
|
neil
:
review-
|
Details | Diff | Splinter Review |
Bug 254714 exist long time ago and now fixed. This bug for porting it to Seamonkey. Thanks to Philip Chee for suggestion.
Flags: wanted-seamonkey2?
Assignee | ||
Comment 1•16 years ago
|
||
This patch does his,job. It needs patch from bug 425480 to be applied first. One thing to make clear - i don't know what to do with gURIFixup removal and make it local variable part from original patch. I need suggestions here.
Attachment #339415 -
Flags: review?(neil)
Assignee | ||
Comment 2•16 years ago
|
||
This one fixed case when user needs hit ESC twice to reset URL in location bar. Also i noticed some good (for me) change in Location bar. When user starts typing - favicon not going to default, but resets to blank. When user hits ESC favicon updated to loaded page's, in contrast of staying default.
Attachment #339415 -
Attachment is obsolete: true
Attachment #339424 -
Flags: review?(neil)
Attachment #339415 -
Flags: review?(neil)
Updated•16 years ago
|
Assignee: nobody → misak
Status: NEW → ASSIGNED
Component: General → Location Bar
Depends on: 254714
OS: Linux → All
QA Contact: general → location-bar
Hardware: PC → All
Assignee | ||
Comment 3•15 years ago
|
||
I've updated patch against current tree with some minor changes.
Attachment #339424 -
Attachment is obsolete: true
Attachment #360694 -
Flags: review?(neil)
Attachment #339424 -
Flags: review?(neil)
Comment 4•15 years ago
|
||
Comment on attachment 360694 [details] [diff] [review] updated patch against current tree >+ if (!gURLBar) >+ return; Don't need this, we always have a URL bar >- if (gBrowser.shouldLoadFavIcon(aURI)) { >- var favStr = gBrowser.buildFavIconString(aURI); >- if (favStr != gProxyFavIcon.src) { >- gBrowser.loadFavIcon(aURI, "src", gProxyFavIcon); >- gProxyDeck.selectedIndex = 0; >- } >- else gProxyDeck.selectedIndex = 1; >- } >- else { >- gProxyDeck.selectedIndex = 0; >- gProxyFavIcon.removeAttribute("src"); >- } >+ >+ PageProxySetIcon(gBrowser.mCurrentBrowser.mIconURL); > } else if (aState == "invalid") { > gURLBar.removeEventListener("input", UpdatePageProxyState, false); >- gProxyDeck.selectedIndex = 0; >+ PageProxyClearIcon(); Not sure what the point of these changes is. >+ if (gURLBar && >+ gURLBar.value == "" && >+ getWebNavigation().currentURI.spec == "about:blank") >+ URLBarSetURI(uri); Won't this set the page proxy state to valid? Not sure that's a good thing, since there are some cases where we never get a location change (which is what this code is trying to work around, I guess.)
Comment 5•15 years ago
|
||
>>- if (gBrowser.shouldLoadFavIcon(aURI)) { >>- var favStr = gBrowser.buildFavIconString(aURI); >>- if (favStr != gProxyFavIcon.src) { >>- gBrowser.loadFavIcon(aURI, "src", gProxyFavIcon); >>- gProxyDeck.selectedIndex = 0; >>- } >>- else gProxyDeck.selectedIndex = 1; >>- } >>- else { >>- gProxyDeck.selectedIndex = 0; >>- gProxyFavIcon.removeAttribute("src"); >>- } >>+ >>+ PageProxySetIcon(gBrowser.mCurrentBrowser.mIconURL); >> } else if (aState == "invalid") { >> gURLBar.removeEventListener("input", UpdatePageProxyState, false); >>- gProxyDeck.selectedIndex = 0; >>+ PageProxyClearIcon(); > Not sure what the point of these changes is. The tabbrowser code already does*almost* all that the deleted code does when it updates .mIconURL. So here we just catch a free ride by using whatever is in .mIconURL. I say *almost* because see below: >+function PageProxySetIcon (aURL) >+{ >+ if (!gProxyFavIcon) >+ return; >+ >+ if (!aURL) >+ PageProxyClearIcon(); >+ else if (gProxyFavIcon.getAttribute("src") != aURL) >+ gProxyFavIcon.setAttribute("src", aURL); >+} >+ >+function PageProxyClearIcon () >+{ >+ gProxyFavIcon.removeAttribute("src"); >+} I think you need to update the |gProxyDeck.selectedIndex| in the following: +function PageProxySetIcon(aURL) +{ + if (!gProxyFavIcon) + return; + if (!aURL) { + PageProxyClearIcon(); + return; + } + if (gProxyFavIcon.getAttribute("src") != aURL) + gProxyFavIcon.setAttribute("src", aURL); + if (gProxyDeck.selectedIndex != 1) + gProxyDeck.selectedIndex = 1; +} + +function PageProxyClearIcon() +{ + if (gProxyDeck.selectedIndex != 0) gProxyDeck.selectedIndex = 0; - } + if (gProxyFavIcon.hasAttribute("src")) + gProxyFavIcon.removeAttribute("src"); }
Comment 6•15 years ago
|
||
(In reply to comment #5) > I think you need to update the |gProxyDeck.selectedIndex| in the following: > + if (gProxyFavIcon.getAttribute("src") != aURL) > + gProxyFavIcon.setAttribute("src", aURL); > + if (gProxyDeck.selectedIndex != 1) > + gProxyDeck.selectedIndex = 1; > + if (gProxyDeck.selectedIndex != 0) > gProxyDeck.selectedIndex = 0; > + if (gProxyFavIcon.hasAttribute("src")) > + gProxyFavIcon.removeAttribute("src"); These checks don't seem to make much sense. At least, I would expect the underlying code to deal with such things smartly...
Comment 7•15 years ago
|
||
> These checks don't seem to make much sense. At least, I would expect the
> underlying code to deal with such things smartly...
Yes, you are right. This is from a very bit rotted patch I wrote a long time ago when I knew less.
Assignee | ||
Comment 8•15 years ago
|
||
patch from this bug moved to bug 425480, Neil suggested
Depends on: 425480
Assignee | ||
Comment 9•15 years ago
|
||
Remaining bits after landing of bug 425480.
Attachment #360694 -
Attachment is obsolete: true
Attachment #362870 -
Flags: review?
Attachment #360694 -
Flags: review?(neil)
Assignee | ||
Comment 10•15 years ago
|
||
Oops, forgone one line.
Attachment #362870 -
Attachment is obsolete: true
Attachment #362871 -
Flags: review?(neil)
Attachment #362870 -
Flags: review?
Comment 11•15 years ago
|
||
Comment on attachment 362871 [details] [diff] [review] proper patch I don't think application/octet-stream is the proper mime-type for a patch.
Attachment #362871 -
Attachment is patch: true
Attachment #362871 -
Attachment mime type: application/octet-stream → text/plain
Comment 12•15 years ago
|
||
Comment on attachment 362871 [details] [diff] [review] proper patch Our page proxy favicon works in a completely different way to Firefox's, so none of these changes makes sense.
Attachment #362871 -
Flags: review?(neil) → review-
Comment 13•15 years ago
|
||
> (From update of attachment 362871 [details] [diff] [review])
> Our page proxy favicon works in a completely different way to Firefox's, so
> none of these changes makes sense.
These changes look reasonable to me. Why don't they make sense to you?
Comment 14•15 years ago
|
||
Comment on attachment 362871 [details] [diff] [review] proper patch >- gProxyButton.setAttribute("pageproxystate", aState); >+ gURLBar.setAttribute("pageproxystate", aState); >+ gProxyFavIcon.setAttribute("pageproxystate", aState); This change is definitely wrong, but maybe I misread the rest of the patch.
Comment 15•15 years ago
|
||
Since bug 254714 was ported in bug 425480 can you please resummarise this bug to whatever it is you're trying to do here?
Comment 16•15 years ago
|
||
Comment on attachment 362871 [details] [diff] [review] proper patch >+ PageProxySetIcon(gBrowser.mCurrentBrowser.mIconURL); Unfortunately mIconURL won't be set in the case of a /favicon.ico icon so this would break those completely if they hadn't already been broken already.
Comment 17•15 years ago
|
||
Aargh, so we have two paths. onLinkAdded calls setIcon which updates mIconURL. Then you have mTabProgressListener::onStateChange() which calls loadFavIcon() which updates the corresponding tab if mIconURL is empty. And it's loadFavIcon() which checks for /favicon.ico. What should we do here?
Updated•15 years ago
|
Summary: port Bug 254714 to SeaMonkey → port bug 254714 to SeaMonkey (blank location bar while loading page in a new tab/window)
Comment 18•15 years ago
|
||
From comments here, it's unclear if this is still needed or not. I don't see any reason to give this special priority, but it's also not unwanted, so cancelling the wanted flag.
Flags: wanted-seamonkey2.0?
Updated•13 years ago
|
Target Milestone: seamonkey2.0 → ---
Comment 19•12 years ago
|
||
> From comments here, it's unclear if this is still needed or not.
Bump?
Comment 20•12 years ago
|
||
>> From comments here, it's unclear if this is still needed or not. > Bump? Did you try out the STR in Bug 254714?
Comment 21•12 years ago
|
||
Yes, I can't reproduce the issue
Comment 22•12 years ago
|
||
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Firefox/17.0 SeaMonkey/2.14a1 ID:20120717003004 CSet: fc60215fa2fa When following the STR in bug 254714 comment #58, I cannot reproduce the issue. Nor can I reproduce it by loading (after opening a new window ;-) ) a homepage of several hundreds of tabs, which used to suffer from the problem in the past.
Updated•12 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•