Last Comment Bug 778392 - Use handleEvent() for BrowserOnClick
: Use handleEvent() for BrowserOnClick
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: Firefox 17
Assigned To: Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-27 20:12 PDT by Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
Modified: 2012-07-31 19:18 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (14.10 KB, patch)
2012-07-28 02:05 PDT, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
felipc: review+
Details | Diff | Splinter Review
patch ver.2 (12.23 KB, patch)
2012-07-30 21:11 PDT, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
felipc: review+
Details | Diff | Splinter Review

Description Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-07-27 20:12:02 PDT
Use handleEvent() for BrowserOnClick().
If we use handleEvent(), BrowserOnClick will be clearly.
Comment 1 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-07-28 02:05:02 PDT
Created attachment 646826 [details] [diff] [review]
patch
Comment 2 Josh Matthews [:jdm] 2012-07-28 07:01:31 PDT
Comment on attachment 646826 [details] [diff] [review]
patch

I am not a browser peer, so I'm going to randomly select Felipe.
Comment 3 :Felipe Gomes (needinfo me!) 2012-07-30 15:23:53 PDT
Comment on attachment 646826 [details] [diff] [review]
patch

Review of attachment 646826 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Ohzeki, this is some great clean-up. If you could please update the patch with the style nits below fixed (and the `safebrowsing` change) I'll land it.

::: browser/base/content/browser.js
@@ +2528,5 @@
>            Components.utils.reportError("Couldn't get ssl_override pref: " + e);
>          }
>  
>          window.openDialog('chrome://pippki/content/exceptionDialog.xul',
> +            '','chrome,centerscreen,modal', params);

this change (here and elsewhere) to the indentation style is incorrect. Multiple line of parameters should all be indented past the opening parens, like it was before

@@ +2600,5 @@
> +        Ci.nsIPermissionManager.EXPIRE_SESSION);
> +
> +    let buttons = [{
> +      label: gNavigatorBundle.getString("safebrowsing.getMeOutOfHereButton.label"),
> +        accessKey: gNavigatorBundle.getString("safebrowsing.getMeOutOfHereButton.accessKey"),

broken indent

@@ +2611,5 @@
> +      buttons[1] = {
> +        label: gNavigatorBundle.getString("safebrowsing.notAnAttackButton.label"),
> +        accessKey: gNavigatorBundle.getString("safebrowsing.notAnAttackButton.accessKey"),
> +        callback: function() {
> +          openUILinkIn(safebrowsing.getReportURL('MalwareError'), 'tab');

note: `safebrowsing` seem to have changed recently to become gSafeBrowsing

(changed in http://hg.mozilla.org/mozilla-central/rev/3b312c31d1cd)

@@ +2757,5 @@
>    _printPreviewTab: null,
>    _tabBeforePrintPreview: null,
>  
>    getPrintPreviewBrowser: function () {
> +    if (!this._printPreviewTab)  {

unrelated change

@@ +6113,5 @@
>          warnAboutClosingTabs = true;
> +      // If the current window is not in private browsing mode we don't need to
> +      // look for other pb windows, we can leave the loop when finding the
> +      // first non-popup window. If however the current window is in private
> +      // browsing mode then we need at least one other pb and one non-popup

unrelated change
Comment 4 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-07-30 21:11:50 PDT
Created attachment 647426 [details] [diff] [review]
patch ver.2

Rebase & fixed.
Comment 5 :Felipe Gomes (needinfo me!) 2012-07-31 09:56:08 PDT
Comment on attachment 647426 [details] [diff] [review]
patch ver.2

Thank you!
Comment 6 :Felipe Gomes (needinfo me!) 2012-07-31 09:56:50 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf51689ab43e
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-07-31 19:18:30 PDT
https://hg.mozilla.org/mozilla-central/rev/cf51689ab43e

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