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
https://hg.mozilla.org/mozilla-central/rev/f173a9a1e056
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: