Skip to content

Bug 1519374 - Stop using gecko-focus in favor or mobile-X workers#579

Merged
JohanLorenzo merged 3 commits intomozilla-mobile:masterfrom
JohanLorenzo:bug-1519374
Feb 14, 2019
Merged

Bug 1519374 - Stop using gecko-focus in favor or mobile-X workers#579
JohanLorenzo merged 3 commits intomozilla-mobile:masterfrom
JohanLorenzo:bug-1519374

Conversation

@JohanLorenzo
Copy link
Copy Markdown
Contributor

@JohanLorenzo JohanLorenzo commented Feb 7, 2019

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features Not applicable

Before merging checklist

  • Changelog: This PR includes a changelog entry or does not need one.

@JohanLorenzo JohanLorenzo requested a review from a team as a code owner February 7, 2019 15:46
@ghost

This comment has been minimized.

1 similar comment
@ghost
Copy link
Copy Markdown

ghost commented Feb 7, 2019

Submitting the task to Taskcluster failed. Details

bad indentation of a mapping entry at line 113, column 21:
command:
^

@ghost

This comment has been minimized.

@ghost

This comment has been minimized.

@JohanLorenzo JohanLorenzo force-pushed the bug-1519374 branch 3 times, most recently from fbd38d6 to f915af8 Compare February 11, 2019 14:01
@JohanLorenzo JohanLorenzo changed the title [WIP] Bug 1519374 - Stop using gecko-focus in favor or mobile-X workers Bug 1519374 - Stop using gecko-focus in favor or mobile-X workers Feb 11, 2019
@JohanLorenzo
Copy link
Copy Markdown
Contributor Author

JohanLorenzo commented Feb 11, 2019

I didn't do mozilla-mobile/fenix@55b419c on this repo, because you still need to clone a repo. We should change the docker image, but that can wait for mozilla-mobile/android-components#1874 to be done.

Copy link
Copy Markdown
Contributor

@mitchhentges mitchhentges left a comment

Choose a reason for hiding this comment

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

Big 🤔 about the refactor pattern, but approved for the underlying change

in:
$let:
decision_worker_type:
$if: 'is_repo_trusted'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Having to split these onto two levels because of how Taskcluster handles variables is 😢
I made a bug for this, but it's stalled

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah :/ I don't think this is a big deal for us, yet. So far, we just need 2 layers. It would become a bigger deal if we had to deal with 4, I think.

Copy link
Copy Markdown
Contributor

@mitchhentges mitchhentges Feb 12, 2019

Choose a reason for hiding this comment

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

It's unnecessary noise that you need to filter out when scanning through the file, and it throws you off: when I see a block is indented, I implicitly assume that it's a bit "different" from less-indented blocks. Though we haven't reached a "staircase" (:stuck_out_tongue:) amount of steps for $let just yet, it's still a little jarring to have to filter out that "yes this is indented, but it's just more var declarations".

There's not much we can do here, so 👍 :)

@JohanLorenzo JohanLorenzo merged commit 683457b into mozilla-mobile:master Feb 14, 2019
@JohanLorenzo JohanLorenzo deleted the bug-1519374 branch February 14, 2019 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants