Closed Bug 720310 Opened 13 years ago Closed 13 years ago

Rename BrowserSetForcedDetector and remove doReload parameter

Categories

(Firefox :: Menus, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 12

People

(Reporter: smontagu, Assigned: smontagu)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
Follow-up to bug 713825 -- see bug 713825 comment 7: "Seems like BrowserSetForcedDetector should be renamed and any calls with !doReload be eliminated. Followup bug?"
Attachment #590644 - Flags: review?
Attachment #590644 - Flags: review? → review?(gavin.sharp)
Blocks: 720312
Comment on attachment 590644 [details] [diff] [review] Patch >--- a/toolkit/content/charsetOverlay.js >+++ b/toolkit/content/charsetOverlay.js >@@ -1,14 +1,14 @@ > function MultiplexHandler(aEvent) > { > MultiplexHandlerEx( > aEvent, > function Browser_SelectDetector(event) { >- BrowserSetForcedDetector(true/*Reload from history*/); >+ BrowserCharsetReload(/*Reload from history*/); > /* window.content.location.reload() will re-download everything */ Nit: Maybe merge those two comments and put them above BrowserCharsetReload()?
In browser.js, why don't you just call BrowserCharsetReload() directly, and avoid adding a useless CharsetReload()? Same question applies to [Browser]SetForcedCharset.
Attached patch Patch v.2 (obsolete) — Splinter Review
Fine with me -- I thought maybe there was some esoteric reason why it was done that way.
Assignee: nobody → smontagu
Attachment #590644 - Attachment is obsolete: true
Attachment #590644 - Flags: review?(gavin.sharp)
Attachment #590815 - Flags: review?(gavin.sharp)
With qrefresh this time
Attachment #590815 - Attachment is obsolete: true
Attachment #590815 - Flags: review?(gavin.sharp)
Attachment #590816 - Flags: review?(gavin.sharp)
This code is too old and crufty to give it any benefit of the doubt :)
Comment on attachment 590816 [details] [diff] [review] Patch v.2 (no, really!) >diff --git a/toolkit/content/charsetOverlay.js b/toolkit/content/charsetOverlay.js > function MultiplexHandler(aEvent) >+ BrowserCharsetReload(/*Reload from history*/); The comment is no longer needed. You'll also need to update comm-central's suite/browser/browser.js accordingly.
Attachment #590816 - Flags: review?(gavin.sharp) → review+
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #6) > Comment on attachment 590816 [details] [diff] [review] > You'll also need to update comm-central's suite/browser/browser.js accordingly. That would be bug 720312
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 724776
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: