Rename BrowserSetForcedDetector and remove doReload parameter

RESOLVED FIXED in Firefox 12

Status

()

Firefox
Menus
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: smontagu, Assigned: smontagu)

Tracking

unspecified
Firefox 12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 590644 [details] [diff] [review]
Patch

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?
(Assignee)

Updated

5 years ago
Attachment #590644 - Flags: review? → review?(gavin.sharp)
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 3

5 years ago
Created attachment 590815 [details] [diff] [review]
Patch v.2

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)
(Assignee)

Comment 4

5 years ago
Created attachment 590816 [details] [diff] [review]
Patch v.2 (no, really!)

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+
(Assignee)

Comment 7

5 years ago
(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
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f173a9a1e056
Target Milestone: --- → Firefox 12
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/f173a9a1e056
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 724776
You need to log in before you can comment on or make changes to this bug.