Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Reactivate HTML Menu unit tests as part of the test suite#5290

Merged
redmunds merged 4 commits intoadobe:masterfrom
lkcampbell:menu-unit-test
Sep 26, 2013
Merged

Reactivate HTML Menu unit tests as part of the test suite#5290
redmunds merged 4 commits intoadobe:masterfrom
lkcampbell:menu-unit-test

Conversation

@lkcampbell
Copy link
Copy Markdown
Contributor

@ghost ghost assigned redmunds Sep 23, 2013
Comment thread test/spec/SpecRunnerUtils.js Outdated
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.

To keep API simpler, make these changes:

  1. Keep only a single API call with original name: createTestWindowAndRun
  2. Add an optional (3rd) parameter something like: function createTestWindowAndRun(spec, callback, options).
  3. New parameter is an Object (not a boolean), so new options can be added without adding any new params.
  4. Since new parameter is optional, then existing calls will not need to be updated. It only needs to be specified on new calls. If options object or options.hasNativeMenus is not specified, default is brackets.nativeMenus.

@redmunds
Copy link
Copy Markdown
Contributor

Done with review. This looks pretty good. Just some API cleanup.

@lkcampbell
Copy link
Copy Markdown
Contributor Author

@redmunds, API changes made.

@lkcampbell
Copy link
Copy Markdown
Contributor Author

Edited description to remove new API functions since they are no longer part of the PR.

Comment thread src/utils/Global.js Outdated
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.

This form is simpler, and it also checks for null and empty string:

    if (params.get("hasNativeMenus")) {
        global.brackets.nativeMenus = (params.get("hasNativeMenus") === "true");
    } else {
        global.brackets.nativeMenus = (!global.brackets.inBrowser && (global.brackets.platform !== "linux"));
    } 

Also, how expensive is the call to params.get("hasNativeMenus")? Should it be stored in a var so it's not called twice?

@redmunds
Copy link
Copy Markdown
Contributor

Second review complete.

@lkcampbell
Copy link
Copy Markdown
Contributor Author

@redmunds, code updated.

@redmunds
Copy link
Copy Markdown
Contributor

Looks good. Merging.

redmunds added a commit that referenced this pull request Sep 26, 2013
Reactivate HTML Menu unit tests as part of the test suite
@redmunds redmunds merged commit 36b2e4a into adobe:master Sep 26, 2013
@lkcampbell lkcampbell deleted the menu-unit-test branch September 27, 2013 13:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants