Last Comment Bug 720310 - Rename BrowserSetForcedDetector and remove doReload parameter
: Rename BrowserSetForcedDetector and remove doReload parameter
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Menus (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 12
Assigned To: Simon Montagu :smontagu
:
Mentors:
Depends on: 713825 724776
Blocks: 720312
  Show dependency treegraph
 
Reported: 2012-01-23 00:28 PST by Simon Montagu :smontagu
Modified: 2012-02-06 22:01 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (4.94 KB, patch)
2012-01-23 00:28 PST, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
Patch v.2 (4.92 KB, patch)
2012-01-23 12:05 PST, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
Patch v.2 (no, really!) (5.02 KB, patch)
2012-01-23 12:07 PST, Simon Montagu :smontagu
gavin.sharp: review+
Details | Diff | Splinter Review

Description Simon Montagu :smontagu 2012-01-23 00:28:53 PST
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?"
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2012-01-23 00:58:54 PST
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()?
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-23 11:51:45 PST
In browser.js, why don't you just call BrowserCharsetReload() directly, and avoid adding a useless CharsetReload()?

Same question applies to [Browser]SetForcedCharset.
Comment 3 Simon Montagu :smontagu 2012-01-23 12:05:08 PST
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.
Comment 4 Simon Montagu :smontagu 2012-01-23 12:07:02 PST
Created attachment 590816 [details] [diff] [review]
Patch v.2 (no, really!)

With qrefresh this time
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-23 12:36:03 PST
This code is too old and crufty to give it any benefit of the doubt :)
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-23 12:46:36 PST
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.
Comment 7 Simon Montagu :smontagu 2012-01-23 23:11:44 PST
(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
Comment 9 Simon Montagu :smontagu 2012-01-24 11:17:55 PST
https://hg.mozilla.org/mozilla-central/rev/f173a9a1e056

Note You need to log in before you can comment on or make changes to this bug.