935259 - Follow-up: Split DOMLinkAdded switch statement into methods

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

The DOMLinkAdded switch statement of browser.js in Tab.handleEvent now handles favicons, feeds, and opensearch. This is huge, and should just be split up into smaller methods.

Comment 1

[:Margaret Leibovic]

Chenxia, this sounds like it could be a good mentor bug, if you're not planning to fix it in the near future.

Comment 2

[Chenxia Liu [:liuche]]

Good call, I'll add it to the mentoring list.

Comment 3

[James]

Hi, I am new to Firefox and would like to contribute to this bug.

How can I help?

Comment 4

[Chenxia Liu [:liuche]]

Hi James, glad to hear you're interested in helping out with this bug!

First of all, you'll need to have a local Firefox build, so you can write patches and try them out. This is our main wiki for getting started, so go take a look if you need somewhere to start: https://wiki.mozilla.org/Mobile/Fennec/Android

It also helps to have a physical Android device (unfortunately, developing with the emulator is extremely slow).

Once you get a build working, you'll want to start looking at mobile/android/chrome/content/browser.js, at the DOMLinkAdded part of the switch statement. This bug is to simply move the smaller cases into separate functions, because the amount of code there is making that switch statement less readable.

Let me know if you have any questions! (Feel free to "Need more information" me, and I'll be faster about turnaround time.)

Comment 5

[James]

Okay, I've been trying to get my local build working.

I've downloaded the sdk and ndk.

Then I mercurial pulled the repo as on the wiki and when I tried to "./mach build" it didn't work.

I then tried to git clone the repo and again, "./mach build" didn't work.

The error that I'm getting is the exact same one as in https://bugzilla.mozilla.org/show_bug.cgi?id=908296

I'm on the #build IRC and I haven't had anyone who can help me yet.

Do you know of anyway to help me please?

Comment 6

[:Margaret Leibovic]

Since you're getting the same error that's in bug 908296, maybe gps would know how to help?

Comment 7

[James]

I managed to fix the bug.

Turns out that I needed to remove pyutil and reinstall it along with a few other libraries.

How long should it take to run "./mach build"?

It's taken my computer over two hours so far.

Is that normal for a first time build?

Comment 8

[:Margaret Leibovic]

(In reply to James from comment #7)
> I managed to fix the bug.
> 
> Turns out that I needed to remove pyutil and reinstall it along with a few
> other libraries.
> 
> How long should it take to run "./mach build"?
> 
> It's taken my computer over two hours so far.
> 
> Is that normal for a first time build?

That seems like an unusually long time :/ Did it finish eventually? What are the specs of your machine?

Comment 9

[James]

It seems that was just a once off.

Now when I compile it only takes about five minutes now.

I've successfully installed it on my phone though.

It's a two year old laptop with an Intel P6200, so not great.

I can finally start working on the code.

Comment 10

[James]

Do you want me to move all of the if statements into their own methods?

Once I've finished, how do I push my code back?

I cloned the github repo by the way.

Comment 11

[:Margaret Leibovic]

(In reply to James from comment #10)
> Do you want me to move all of the if statements into their own methods?

Yes, I think we should move the bodies of the if statements into new methods (e.g. handleLinkIcon/handleLinkAlternate/handleLinkSeach, or something like that).

> Once I've finished, how do I push my code back?

You should upload a patch to this bug, and you can set the review? flag to me (Chenxia is on vacation, so I can handle the review). See this MDN page for some helpful details on creating a patch:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

> I cloned the github repo by the way.

That link has some details about generating an hg-friendly patch from git. If you have trouble, you can ask in #mobile on IRC.

Comment 12

[Quentin BOUILLAGUET]

Attachment: 935259_patch_v1.diff

Hi. I follow all the instructions on this thread and manage to split the code in DOMLinkAdded into 4 methods.

Comment 13

[:Margaret Leibovic]

Thanks for writing a patch! In the future, you should make sure to set the review? or feedback? flag on a patch to make sure it doesn't get lost in the shuffle. I'll do that for you now :)

Comment 14

[Quentin BOUILLAGUET]

Thank you for setting the correct flag ! =)

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Margaret, I am with the student who wrote this patch (I am doing a presentation of Firefox, Mozilla and our workflow).
Do you know someone else who could review it? (if the patch lands, they will get a good grade from their teacher :)

Comment 16

[Chenxia Liu [:liuche]]

Sorry about that! I'll make sure to have this reviewed by the end of today.

Comment 17

[Chenxia Liu [:liuche]]

Comment on attachment 8567842 [details] [diff] [review]
935259_patch_v1.diff

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

Nice work. Some improvements on method names and arguments, because we want these names to make sense to someone who is just reading the methods. For a refactor, where no changes to the overall logic are occurring, we really want to make sure that methods/variables/arguments are completely clear.

Looks good! Flag me for review again once you've made these changes - I promise I'll be faster about turnaround time this time, sorry!

::: mobile/android/chrome/content/browser.js
@@ +3875,4 @@
>      return viewport;
>    },
>  
> +

Remove these extra newline additions.

@@ +3963,4 @@
>      }
>    },
>  
> +  sanitizeRelString: function(tRel) {

Rename the argument to linkRel instead of tRel. tRel is not very informative, we can drop the reference to "target". For someone reading this function, we want them to be able to easily figure out what the argument is supposed to be.

@@ +3976,5 @@
> +    }
> +    return list;
> +  },
> +
> +  handleLinkIcon: function(tTarget) {

We don't need link in the function name, could be makeFaviconMessage.
Rename this argument to be more specific and useful, to linkTarget or eventTarget.

@@ +3987,5 @@
> +      let sizes = tTarget.getAttribute("sizes").toLowerCase();
> +
> +      if (sizes == "any") {
> +        // Since Java expects an integer, use -1 to represent icons with sizes="any"
> +        maxSize = -1; 

Since you're modifying these lines anyways, there's a trailing space on this line that should be removed.

@@ +4006,5 @@
> +      mime: tTarget.getAttribute("type") || ""
> +    };
> +  },
> +
> +  handleLinkSearch: function(tTarget) {

Better method name, makeOpenSearchMessage, and better argument name, linkTarget or eventTarget, whichever you decide using from the previous comment.

@@ +4061,5 @@
> +      });
> +    }
> +  },
> +
> +  handleLinkAlternate: function(tTarget, elType) {

Nit: move this above the other functions to the order where it's called.

Better naming for arguments. Also better naming for the method, maybe makeFeedMessage.

@@ +4169,4 @@
>  
>        case "DOMLinkAdded":
>        case "DOMLinkChanged": {
> +        var json = false;

Use let not var. Also, name this jsonMessage, and set it equal to null, not false.

@@ +4177,4 @@
>          // Ignore on frames and other documents
>          if (target.ownerDocument != this.browser.contentDocument)
>            return;
> +       

Remove trailing spaces.

@@ +4177,5 @@
>          // Ignore on frames and other documents
>          if (target.ownerDocument != this.browser.contentDocument)
>            return;
> +       
> +        // Sanitize target.rel

Make comment more descriptive and useful: Sanitize rel link.

@@ +4193,2 @@
>          } else if (list.indexOf("[search]" != -1) && aEvent.type == "DOMLinkAdded") {
> +          json = this.handleLinkSearch(target); 

Remove the trailing space.

@@ +4195,4 @@
>          }
> +        if (!json)
> +         return;
> +        

There are trailing spaces here, they should be removed.

Comment 18

[Stéphane SCHMIDELY]

Attachment: patch_browserJS.diff

Thank you for taking the time to review our code. We made the changes you requested us.

We also have removed all the trailing spaces in this file.

Comment 19

[Stéphane SCHMIDELY]

Attachment: patch.diff

The last patch removes all the trailing spaces in the browser.js file. 

This new patch contains only the modifications needed for this bug. I will create another patch which will remove all the trailing spaces in the file.

Comment 20

[Chenxia Liu [:liuche]]

Comment on attachment 8579432 [details] [diff] [review]
patch.diff

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

Thanks, Stéphane! This looks good. One last thing - the commit message of this patch should be:
Bug <#> - <bug name>. r=<reviewer>

so for this bug:
Bug 935259: Follow-up: Split DOMLinkAdded switch statement into methods. r=liuche

Also, I don't know who should be set as the author of this patch. You can update that with 'hg qref --u <user>' which should be of the format 'name <email>'. See the meta info for this patch: https://hg.mozilla.org/integration/fx-team/rev/3019cfbbfd0d

Looks good! Just need to update the meta-info. (I can also do this for you if you want to let me know who the author should be.)

Comment 21

[Chenxia Liu [:liuche]]

Also, no need to write another patch that removes the trailing spaces from the file - in general, if you're touching lines of a file, it's good to clean up trailing spaces because you're changing the author annotation already, but removing whitespace for a line that you're not touching will change the author even without meaningful changes, which makes it difficult for people to find who to contact if they have questions about a piece of code.

So just the first patch you've already uploaded is fine, as long as you update the patch message.

Comment 22

[Stéphane SCHMIDELY]

Attachment: patch.diff

We worked together on this patch. Is it possible to credit bogato too? 

We committed the file with the message that you had given us and after, we did:

> hg export . > patch.diff

Is it what you expected?

Comment 23

[Sylvestre Ledru [:Sylvestre]]

(In reply to Stéphane SCHMIDELY from comment #22)
> Created attachment 8579447 [details] [diff] [review]
> patch.diff
> 
> We worked together on this patch. Is it possible to credit bogato too? 
You can do that in the commit.

> We committed the file with the message that you had given us and after, we
> did:
> 
> > hg export . > patch.diff
> 
> Is it what you expected?
Yes, it is what she expected.

You don't need to request the review again as liuche already accepted the patch.
You can add the keyword "checkin-needed" to have a sheriff landing the change for you.

Comment 24

[Chenxia Liu [:liuche]]

Comment on attachment 8579447 [details] [diff] [review]
patch.diff

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

Thank you, this is good, I'll push this to try and add the checkin-needed flag so it will get landed.

Comment 25

[Chenxia Liu [:liuche]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cce09e665694

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/0b8c71d3aac0

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0b8c71d3aac0