926579 - Find ways to remove duplication between the getWindowsFragment and getTabsFragment function of RecentlyClosedTabsAndWindowsMenuUtils

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

To fix this bug, you'll need to clone the UX branch which is located at https://hg.mozilla.org/projects/ux/.

Both of the functions in RecentlyClosedTabsAndWindowsMenuUtils.jsm perform pretty similar operations, and so there may be some duplicate work that the functions are doing. We should look in to refactoring these functions to remove duplication where possible.

Comment 1

[Aveek Mondal]

Hi, Can I work on this bug? I have already built mozilla-central repository. Now is it possible to update diff for project UX?

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Aveek Mondal from comment #1)
> Hi, Can I work on this bug? I have already built mozilla-central repository.
> Now is it possible to update diff for project UX?

You can work on this bug, but you won't be able to simply update your mozilla-central repository with the UX changes. You should instead clone and build the UX repository at https://hg.mozilla.org/projects/ux/.

Comment 3

[Aveek Mondal]

Hi Jared, Thanks a lot. I have cloned the repository and located the file also. Now if I make any change in this file how can I test that? Please guide me as I am totally new here.

Aveek

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

After you make changes to the file, you can run `./mach build browser/components/sessionstore` and then `./mach run` to run the browser. To run the sessionstore tests you can run `./mach mochitest-browser browser/components/sessionstore/test/`.

You should join #introduction on irc.mozilla.org to get more detailed help.

Comment 5

[Mike Hoye [:mhoye]]

Hi, Aveek - Are you still working on this?

Comment 6

[Mike Conley (:mconley) (:⚙️)]

(In reply to Mike Hoye [:mhoye] from comment #5)
> Hi, Aveek - Are you still working on this?

Sounds like not.

Comment 7

[Harithaki]

Hi,
I'm new to open source projects. I thought of contributing to Mozilla and like to work with this bug. I have already built Firefox source code in windows. I went to the link mentioned above but I have no idea how to proceed. As this is my first bug can you tell me how to proceed with this?

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

The file is located at /browser/components/sessionstore/RecentlyClosedTabsAndWindowsMenuUtils.jsm

Inside of the file you will find two functions that look very similar, getTabsFragment and getWindowsFragment.

The goal of this bug is to reduce duplication between those two functions by pulling out similar code to some shared functions.

Comment 9

[Harithaki]

Thanks a lot.I'm starting to work with it.

Comment 10

[rambo1177]

After cloning the UX branch, how would I go about building the mozilla-central along with the changes because of the branch?

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

There is no UX branch anymore. You should clone mozilla-central at https://hg.mozilla.org/mozilla-central.

Then you can run `./mach build`. Follow the instructions at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build to get started.

Comment 12

[Harithaki]

I have created a new function by pulling out the similar parts in the two functions. I built it and ran the code. But I want to test the functionality of the parts I have changed. How can I do that?

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Harithaki from comment #12)
> I have created a new function by pulling out the similar parts in the two
> functions. I built it and ran the code. But I want to test the functionality
> of the parts I have changed. How can I do that?

1. Open up some windows and go to some web pages in them. Now close those windows. Open up some tabs and go to some webpages in them. Now close those tabs.
2. Open the Firefox menu and click on the History button
3. Look at the Recently Closed Windows and Recently Closed Tabs in the menu.

The items in the menu should match the windows and tabs that you just closed.

Comment 14

[Harithaki]

Do I have to submit only the .patch file?

Comment 15

[Harithaki]

Attachment: Removing Duplication in getTabsFragment and getWindowsFragment and adding them to a different function getFragment.

I have added a patch file. Can I know what are the further changes I should add to it?

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8774114 [details] [diff] [review]
Removing Duplication in getTabsFragment and getWindowsFragment and adding them to a different function getFragment.

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

::: browser/components/sessionstore/RecentlyClosedTabsAndWindowsMenuUtils.jsm
@@ +119,5 @@
>        for (let i = 0; i < closedTabs.length; i++) {
>          let element = doc.createElementNS(kNSXUL, aTagName);
>          element.setAttribute("label", closedTabs[i].title);
> +        
> +		let element = getFragment:getFirst(aTagName, closedTabs[i], element, isTab = true, i);

Did you test this? I've never seen this type of syntax in JavaScript before? Please test your patch to make sure that it works before requesting review.

Once you have the source downloaded, you can run `mach build` to build the browser. If you enable artifact builds it will compile much faster (see https://developer.mozilla.org/en-US/docs/Artifact_builds).

Comment 17

[Harithaki]

I did 'mach build' and 'mach run' and tested by opening and closing some tabs and windows and checking whether they are in the recently closed tabs and recently closed windows in the menu. I think I'm testing in a wrong way. I'll look in to it again.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Okay, please test again. When you do `mach run`, you should do the following:

1. Open the developer Tools (Ctrl+Shift+I on Windows)
2. Click the gear icon in the top row on the end of the toolbar (just a few buttons before the X)
3. Scroll down in the options and check the boxes for:
   Enable browser chrome and add-on debugging toolboxes
   Enable remote debugging
4. Close the developer tools
5. Open the Browser Toolbox (Firefox menu > Developer > Browser Toolbox)
6. In the Browser Toolbox, choose the Debugger
7. Once all scripts are loaded, search for RecentlyClosedTabsAndWindowsMenuUtils.jsm
8. Confirm that the changes you made to the file exist in the source code being shown by the debugger.

Alternately, you could load resource:///modules/sessionstore/RecentlyClosedTabsAndWindowsMenuUtils.jsm through the address bar and view the source code there. It will be better for you to learn how to use the debugger though.

Comment 19

[Harithaki]

Sorry to trouble you again. I checked it and it's taking the changed code. I tried several things but still in the recently closed Tabs and Windows it shows the closed tabs and windows correctly. Are there some other ways where I can check this codes functionality?

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Yes, you can put a call to `Services.console.logStringMessage("test message");` in your code and see if it shows up in the Browser Console (Ctrl+Shift+J on Windows). If you don't see that message when you open up the menu then it's not running your code.

Also, please use the "needinfo" feature at the bottom of this page if you have a question for me as I may not notice the specific bugzilla comment otherwise.

Comment 21

[Harithaki]

The message is not shown, is it because of some problem with the build?

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Yes, there is a problem with your local build. You should join the #introduction channel on Mozilla's IRC server (irc.mozilla.org) to get some guided help in getting your build working.

Comment 23

[Wasim Thabraze]

Hello Jared,

I would like to work on this bug. I have experience of resolving couple of bugs in Bugzilla-Mozilla.

I feel glad if you can mentor me in fixing it.

Thank You.

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Hello Wasim, thank you for volunteering to work on this bug. Please let me know if you have any questions on how to fix this bug and get your build up and running. See comment #8 for details on how to fix this bug. I will assign the bug to you after you have uploaded a patch.

Comment 25

[David Malaschonok]

Attachment: Bug 926579 - Remove duplication in RecentlyClosedTabsAndWindowsMenuUtils through refactoring.

Review commit: https://reviewboard.mozilla.org/r/87304/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87304/

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8803080 [details]
Bug 926579 - Remove duplication in RecentlyClosedTabsAndWindowsMenuUtils through refactoring.

https://reviewboard.mozilla.org/r/87304/#review86572

This looks really great! Please make the following changes and re-push to mozreview and I'll land this for you. Thanks!

There are a few lines that have trailing whitespace that got introduced with this patch. Please remove all trailing whitespace in this file.

::: browser/components/sessionstore/RecentlyClosedTabsAndWindowsMenuUtils.jsm:115
(Diff revision 1)
>  function setImage(aWindow, aItem, aElement) {
>    let iconURL = aItem.image;
>    // don't initiate a connection just to fetch a favicon (see bug 467828)
>    if (/^https?:/.test(iconURL))
>      iconURL = "moz-anno:favicon:" + iconURL;
>  
>    aElement.setAttribute("image", iconURL);
>  }

note, aWindow is unused in this function so it can be removed from the list of arguments.

::: browser/components/sessionstore/RecentlyClosedTabsAndWindowsMenuUtils.jsm:141
(Diff revision 1)
> +function createEntry(aTagName, aIsWindowsFragment, aIndex, aClosedTab,
> +                     aWindow, aMenuLabel, aFragment) {
> +  let doc = aWindow.document;

Since we don't need to pass aWindow to setImage anymore, we can change createEntry to take `document` as the argument instead of `window`. Same for createRestoreAllEntry.

Comment 27

[David Malaschonok]

Attachment: Bug 926579 #2 - Remove unused parameters in RecentlyClosedTabsAndWindowsMenuUtils.

Review commit: https://reviewboard.mozilla.org/r/87702/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87702/

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8803423 [details]
Bug 926579 #2 - Remove unused parameters in RecentlyClosedTabsAndWindowsMenuUtils.

https://reviewboard.mozilla.org/r/87702/#review86702

Thanks! I'll merge these two patches and push them to the fx-team repo. After this lands in fx-team, it will get merged to mozilla-central in a day or two. The next day this change will appear in Firefox Nightly. Thanks for your help and very well written patches!

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 926579 - Remove duplication in RecentlyClosedTabsAndWindowsMenuUtils through refactoring.

Review commit: https://reviewboard.mozilla.org/r/87742/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87742/

Comment 30

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8803485 [details]
Bug 926579 - Remove duplication in RecentlyClosedTabsAndWindowsMenuUtils through refactoring.

https://reviewboard.mozilla.org/r/87742/#review86710

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da1b726ffaee
Remove duplication in RecentlyClosedTabsAndWindowsMenuUtils through refactoring. r=jaws

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da1b726ffaee

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

Thanks for the patch David. Would you like to work on bug 552434 next? I think you'd be well suited since you were able to handle this bug pretty quickly.

Comment 34

[David Malaschonok]

Thank you for pointing me at it. I'll definitely give it a shot.