Last Comment Bug 1323134 - SessionStore.jsm shouldn't set the crop attribute on tabs anymore
: SessionStore.jsm shouldn't set the crop attribute on tabs anymore
Status: RESOLVED FIXED
[good first bug][lang=js]
: good-first-bug
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
-- normal (vote)
: Firefox 53
Assigned To: Vedant Sareen
:
: Mike de Boer [:mikedeboer]
Mentors: Dão Gottwald [:dao]
Depends on: 658467
Blocks:
  Show dependency treegraph
 
Reported: 2016-12-12 22:09 PST by Dão Gottwald [:dao]
Modified: 2016-12-18 20:28 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Diff file displaying the changes made to SessionStore.jsm (674 bytes, patch)
2016-12-17 23:59 PST, Vedant Sareen
dao+bmo: review+
Details | Diff | Splinter Review

Description User image Dão Gottwald [:dao] 2016-12-12 22:09:36 PST
Since bug 658467 landed, the crop attribute doesn't do anything on tabs anymore, so we should stop setting it here:

https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/browser/components/sessionstore/SessionStore.jsm#856
Comment 1 User image Vedant Sareen 2016-12-14 09:49:43 PST

(In reply to Dão Gottwald [:dao] from comment #0)
> Since bug 658467 landed, the crop attribute doesn't do anything on tabs
> anymore, so we should stop setting it here:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/browser/components/sessionstore/
> SessionStore.jsm#856

Dear Sir,

I am new to the open source community, and I wish to start contributing to it.
I saw that this bug has been marked as a good first bug, so I thought I might start here.

From what I understand, all that needs to be done is to see where the crop attribute for a tab is being set (which I believe is at two places), and then remove those lines of code after verifying that they don't affect any other areas of code adversely.

Do I have the right idea? If not, then since the code linked here is large, could you please point me in the right direction about what is to be done, so that I am better able to understand the problem and 
then analyze the code accordingly? 

Thanks.
Comment 2 User image Dão Gottwald [:dao] 2016-12-14 09:51:59 PST
(In reply to Vedant Sareen from comment #1)
> From what I understand, all that needs to be done is to see where the crop
> attribute for a tab is being set (which I believe is at two places), and
> then remove those lines of code after verifying that they don't affect any
> other areas of code adversely.
> 
> Do I have the right idea?

Quite right!
Comment 3 User image Vedant Sareen 2016-12-14 09:56:55 PST
(In reply to Dão Gottwald [:dao] from comment #2)
> Quite right!

May I then start working on it?
Comment 4 User image Dão Gottwald [:dao] 2016-12-14 20:59:55 PST
(In reply to Vedant Sareen from comment #3)
> (In reply to Dão Gottwald [:dao] from comment #2)
> > Quite right!
> 
> May I then start working on it?

Yep, go ahead and let me know if you have questions.
Comment 5 User image Vedant Sareen 2016-12-17 06:30:31 PST
Apologies for the late response.

I have 
1. Configured the tools (Linux build prerequisites) 
2. Downloaded the Mozilla source code via Mercurial
3. Made the changes in the file 'SessionStore.jsm' (Deleted lines 856 and 859 as seen in the file attached in your initial comment)
4. Built Firefox by executing ./mach build
5. Tried testing the changes by executing ./mach mochitest. However, I stopped the test processes as it went on for 4 hours. I didn't notice any failed tests though.

Should I proceed with submitting the patch for review now?
Comment 6 User image Dão Gottwald [:dao] 2016-12-17 08:49:03 PST
You can run only Session Restore tests this way:

./mach mochitest browser/components/sessionstore/
Comment 7 User image Vedant Sareen 2016-12-17 10:28:10 PST
(In reply to Dão Gottwald [:dao] from comment #6)
> You can run only Session Restore tests this way:
> 
> ./mach mochitest browser/components/sessionstore/

I did so now, and the build passed all the tests.
Comment 8 User image Dão Gottwald [:dao] 2016-12-17 11:02:17 PST
Please upload your patch then!
Comment 9 User image Vedant Sareen 2016-12-17 23:59:09 PST
Created attachment 8819637 [details] [diff] [review]
Diff file displaying the changes made to SessionStore.jsm

Proposed patch for Bug 1323134 :)
Comment 10 User image Dão Gottwald [:dao] 2016-12-18 10:07:38 PST
Comment on attachment 8819637 [details] [diff] [review]
Diff file displaying the changes made to SessionStore.jsm

Looks good, thanks!
Comment 11 User image Pulsebot 2016-12-18 10:09:42 PST
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6103077f0ab
Stop setting the crop attribute on tabs. r=dao
Comment 12 User image Phil Ringnalda (:philor) 2016-12-18 20:28:19 PST
https://hg.mozilla.org/mozilla-central/rev/f6103077f0ab

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