Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Fixes tab pages after setting change #9773

Merged
merged 1 commit into from
Jul 11, 2017

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Jun 29, 2017

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Resolves #7806

Auditors: @bsclifton @cezaraugusto

Test Plan:

  • Open about:preferences#tabs
  • Set the number of tabs per tab set to 6
  • Open new tabs to create a new tab set, which has just 2 tabs
  • Increase the number to 8

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

Sorry, something went wrong.

@NejcZdovc NejcZdovc added this to the 0.19.x (Nightly Channel) milestone Jun 29, 2017
@NejcZdovc NejcZdovc self-assigned this Jun 29, 2017
@NejcZdovc NejcZdovc force-pushed the hotfix/#7806-tabpages branch 2 times, most recently from f376e66 to 8797bfc Compare June 29, 2017 12:22
@NejcZdovc NejcZdovc force-pushed the hotfix/#7806-tabpages branch from 8797bfc to 6c27646 Compare June 29, 2017 20:52
@NejcZdovc NejcZdovc modified the milestones: 0.18.x (Developer Channel), 0.19.x (Nightly Channel) Jun 30, 2017
.changeSetting(settings.TABS_PER_PAGE, 1)
.waitForElementCount(tabPage, defaultTabsPerPage)
.waitForBrowserWindow()
.loadUrl('about:preferences')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not loaded in the right context, but I am not sure how to switch to the right one

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tabByIndex?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem is that is not working for me. Most of the times we use if with 0, maybe 1. But my use case is that I need to focus last active tab, which is this case 19 (we have 20 tabs). But if do .tabByIndex(19) second tab is selected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pushed new version with tabByIndex(19) so you can try it out. I am thinking that something is not ok with handles?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test fixed based on @cezaraugusto findings

@NejcZdovc NejcZdovc force-pushed the hotfix/#7806-tabpages branch 2 times, most recently from 0474bde to a0b1a86 Compare June 30, 2017 11:22
@NejcZdovc NejcZdovc force-pushed the hotfix/#7806-tabpages branch 2 times, most recently from fb6a002 to e6daef6 Compare July 9, 2017 07:01

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Resolves brave#7806

Auditors: @bsclifton @cezaraugusto

Test Plan:
- Open about:preferences#tabs
- Set the number of tabs per tab set to 6
- Open new tabs to create a new tab set, which has just 2 tabs
- Increase the number to 8
@NejcZdovc
Copy link
Contributor Author

@cezaraugusto PR is now ready for a review, test was fixed based on your discovery. Thank you

Copy link
Contributor

@luixxiul luixxiul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the test plan works.

@NejcZdovc NejcZdovc merged commit 55aa379 into brave:master Jul 11, 2017
@NejcZdovc
Copy link
Contributor Author

@luixxiul thank you for your test

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants