878691 - Defect - Metro Firefox form history should use async FormHistory.jsm instead of deprecated nsIFormHistory2

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Browser, defect, P3)

Description

[Mark Hammond [:markh] [:mhammond]]

We'd like to remove nsIFormHistory in-favour of the async FormHistory.jsm.  Metro code still uses nsIFormHistory2 - this bug is to replace that usage with the new module.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

- For sanitize.js, you just need to port the changes from http://hg.mozilla.org/mozilla-central/diff/0b21e902f7a0/browser/base/content/sanitize.js (also the followup: http://hg.mozilla.org/mozilla-central/rev/96fe69d53f35).

(And we really need to unfork these files - Android also has a copy in mobile/android/modules/Sanitizer.jsm that also needs to be fixed in bug 878692 :( )

- Also similar to android, the browser-ui.js reference to nsIFormHistory2 should just removed, since it isn't doing anything useful (form history initialization was refactored in bug 566746, it initializes itself now).

- Updating http://hg.mozilla.org/mozilla-central/annotate/8b1bfcf0ce6e/browser/metro/base/tests/mochitest/browser_form_auto_complete.js#l8 to use the FormHistory.jsm API is relatively straightforward, just need to make the same call as in sanitize.js (http://hg.mozilla.org/mozilla-central/annotate/8b1bfcf0ce6e/browser/base/content/sanitize.js#l259)

Comment 4

[Matt Brubeck (:mbrubeck)]

If any new contributors would like to work on this bug, it should be pretty straightforward.  See the links to specific source files in comment 1, and follow the instructions to build and run Metro Firefox here:

https://wiki.mozilla.org/Firefox/Windows_8_Integration

Comment 5

[Eklavya Mirani]

Hi,

I have started working on this, kindly assign it to me.

When I run the metro chrome tests, is it supposed to crash in the end? (I see a crashinjectdll.dll causing this)

If this is the case how do I test the changes I made?

Eklavya

Comment 6

[Matt Brubeck (:mbrubeck)]

(In reply to eklavyamirani from comment #5)
> When I run the metro chrome tests, is it supposed to crash in the end? (I
> see a crashinjectdll.dll causing this)

This happens if the tests hang or take too long.  (The intentional crash produces a stack track that help diagnose problems in these cases.)  There are known problems with running mochitest-metro-chrome in debug builds that might cause this.

> If this is the case how do I test the changes I made?

Try running the browser_form_auto_complete.js test to see if that one can run without timing out.  You may or may not need a non-debug build to make this work.  In the top-level directory of your mozilla-central clone:

./mach mochitest-metro browser/metro/base/tests/mochitest/browser_form_auto_complete.js

You can also test manually on a page with a simple form to make sure that saving, auto-filling, and clearing of form history is working.

Comment 7

[Eklavya Mirani]

Attachment: Modified code to use async FormHistory.jsm instead of nsIFormHistory

I have tested this (cleared history and added new entries) manually and automatically (using the browser_form_auto_complete.js) and it is working. Kindly review :)

Comment 8

[Matt Brubeck (:mbrubeck)]

Comment on attachment 796045 [details] [diff] [review]
Modified code to use async FormHistory.jsm instead of nsIFormHistory

Thanks for the patch!  Note: When attaching a patch, you can request review by setting the "review" flag to "?" and then entering a reviewer's name or email address.  I'll take a look at this one right away.

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 796045 [details] [diff] [review]
Modified code to use async FormHistory.jsm instead of nsIFormHistory

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

Thanks again!  The code looks good; there are just some straightforward bugs to fix and some style issues to make it more consistent with our codebase.  See below for details.  You can update your patch to address these comments, then attach the new version to this bug and request review again.

::: browser/metro/base/content/browser-ui.js
@@ +8,5 @@
>  Cu.import("resource://gre/modules/devtools/dbg-server.jsm")
>  
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this,"FormHistory","resource://gre/modules/FormHistory.jsm");
> +

Please move this into browser-scripts.js.  (No functional difference; it's just a file we use to keep all our module imports in one place.)  You can delete the XPCOMUtils line since it's already imported there.

@@ +145,5 @@
>        window.removeEventListener("UIReadyDelayed",  delayedInit, false);
>  
>        // Login Manager and Form History initialization
>        Cc["@mozilla.org/login-manager;1"].getService(Ci.nsILoginManager);
> +	  

nit: please remove the whitespace characters from this line.

::: browser/metro/base/content/sanitize.js
@@ +22,3 @@
>    {
> +    let canClear = this.items[aItemName].canClear;
> +	if(typeof canclear == "function"){

"canclear" should be "canClear" (with a capital C) -- this line will always return false otherwise, so I'm suprised this code is working.

Style nit: Please use a space after keywords like "if", purely for consistency:
https://wiki.mozilla.org/Firefox/Windows_8_Metro_Style_Guides

@@ +24,5 @@
> +	if(typeof canclear == "function"){
> +	  canClear(aCallback, aArg);
> +	  return false;
> +	}
> +	aCallback(aItemname, canClear, aArg);

capital N in aItemName

@@ +26,5 @@
> +	  return false;
> +	}
> +	aCallback(aItemname, canClear, aArg);
> +	return canClear;
> +	

nit: no blank line here, please.

@@ +54,5 @@
>          // rather than fail fast.
>          // Callers should check returned errors and give user feedback
>          // about items that could not be sanitized
> +		
> +		let clearCallback = (itemName, aCanClear) => {

Nit: These lines are indented a bit too far.

@@ +55,5 @@
>          // Callers should check returned errors and give user feedback
>          // about items that could not be sanitized
> +		
> +		let clearCallback = (itemName, aCanClear) => {
> +		  let item = this.items[itemName];

This line is a duplicate of a line above -- you can remove this one.

@@ +65,5 @@
> +		  catch(er){
> +		    if(!errors){
> +			  errors = {};
> +			}
> +			errors[itemName] = er;

The indentation doesn't match the structure here.

@@ +212,3 @@
>        {
> +	    let count = 0;
> +		let countDone = {

Indentation is off again here.  (Sorry, we ought to have a tool to check these style issues automatically but we don't yet.)

@@ +212,5 @@
>        {
> +	    let count = 0;
> +		let countDone = {
> +		  handleResult : 
> +		    function(aResult){

Style nit: Please put "handleResult: function(aResult) {" all on one line, with a space before the "{", just for consistency with the surrounding code.

@@ +227,5 @@
> +		};
> +		
> +		FormHistory.count({}, countDone);
> +		
> +        return false;		

The return value from this function is never used, so we should just remove it.  (I know this isn't your fault, since the desktop Firefox patch had the same problem.)

::: browser/metro/base/tests/mochitest/browser_form_auto_complete.js
@@ +5,5 @@
>  
>  "use strict";
>  
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this,"FormHistory","resource://gre/modules/FormHistory.jsm");

Since this file is loaded in the same window as browser-scripts.js and browser-ui.js, you should be able to remove both of these lines.

Comment 10

[Eklavya Mirani]

Attachment: Updated based on the review recieved. Indentation fixed, bugs corrected.

PCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",           resource://gre/modules/PlacesUtils.jsm");

This line is also present in browser-scripts.js, so assuming it is loaded in the same window I removed it from sanitize.js as well. Please correct me if I am wrong here.

Eklavya

Comment 11

[Matt Brubeck (:mbrubeck)]

Comment on attachment 796646 [details] [diff] [review]
Updated based on the review recieved. Indentation fixed, bugs corrected.

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

Thank you, this looks good!  There are a couple of minor issues, but they are trivial so I will fix them myself and re-test the patch before checking it in for you.  (No need to upload a new patch.)

::: browser/metro/base/content/sanitize.js
@@ +40,5 @@
>    {
>      var branch = Services.prefs.getBranch(this._prefDomain);
>      var errors = null;
>      for (var itemName in this.items) {
> +        if ("clear" in item && item.canClear && branch.getBoolPref(itemName)) {

The "&& item.canClear" part can be removed here. (It is replaced by the aCanClear check below.)

@@ +61,5 @@
> +                dump("Error sanitizing " + itemName + ":" + er + "\n");
> +              }
> +            }
> +          }
> +      this.canClearItem(itemName, clearCallback);

This should be inside of the "if" statement.  (It should be moved just before the last "}" above.)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 796646 [details] [diff] [review]
Updated based on the review recieved. Indentation fixed, bugs corrected.

In the future, you can just use the checkin-needed bug keyword.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/747b66a08641

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 796646 [details] [diff] [review]
Updated based on the review recieved. Indentation fixed, bugs corrected.

Got our wires a bit crossed there. This wasn't ready to land yet. Backed out.
https://hg.mozilla.org/integration/fx-team/rev/0f5c980c9fa1

Comment 15

[Matt Brubeck (:mbrubeck)]

Re-landed with review comments addressed and some whitespace cleanup.  Thanks again!
https://hg.mozilla.org/integration/fx-team/rev/1dec37cb65df

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1dec37cb65df