Skip to content

Refactoring: Turn private custom element methods into functions.#3882

Merged
cramforce merged 2 commits intoampproject:masterfrom
cramforce:simplify-custom-element
Jul 2, 2016
Merged

Refactoring: Turn private custom element methods into functions.#3882
cramforce merged 2 commits intoampproject:masterfrom
cramforce:simplify-custom-element

Conversation

@cramforce
Copy link
Copy Markdown
Member

And inline a few methods.

The calls on the element are slow and cannot be optimized because the compiler doesn't touch the extern objects.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

just for my own knowledge, is it right that this function inherits the same signatures as the method?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No :)

@erwinmombay
Copy link
Copy Markdown
Member

LGTM, 1 question and 1 nit.

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 wouldn't be called when there's an error in the previous code.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch. Fixed.

cramforce added 2 commits July 1, 2016 16:41
And inline a few methods.

The calls on the element are slow and cannot be optimized because the compiler doesn't touch the extern objects.
@cramforce cramforce force-pushed the simplify-custom-element branch from 0027e4f to 1ce6f02 Compare July 1, 2016 23:44
@cramforce
Copy link
Copy Markdown
Member Author

Addressed review comments

@cramforce cramforce merged commit 529574c into ampproject:master Jul 2, 2016
@cramforce cramforce deleted the simplify-custom-element branch July 2, 2016 03:40
jonasmattsson1 added a commit to widespace-os/amphtml that referenced this pull request Jul 5, 2016
* master: (236 commits)
  trim all the columns (ampproject#3894)
  Refactoring: Turn private custom element methods into functions. (ampproject#3882)
  Lower the load priority of ad shaped iframes. (ampproject#3863)
  JsDoc fix (ampproject#3892)
  Add screenshots for Opera to AMP Validator extension. (ampproject#3866)
  Fix renaming of generated JSCompiler_prototypeAlias variable. (ampproject#3887)
  fix typo in amp-sidebar.md (ampproject#3833)
  Validator Roll-up (ampproject#3885)
  [CryptoService] Leverage browser native Crypto API to hash strings. (ampproject#3850)
  Size update (ampproject#3883)
  copy amp-ad docs to builtins (ampproject#3879)
  move doc to extension (ampproject#3878)
  [amp-experiment] Exposes isDismissed() method in AmpUserNotification (ampproject#3832)
  fix action-impl warning on dist (ampproject#3867)
  Add params for microad (ampproject#3827)
  Fixed some A4A tests. (ampproject#3859)
  Updates to colanalytics vendor config for amp-analytics. (ampproject#3849)
  Changes to implement A4A (AMP ads for AMP pages) (ampproject#3534)
  Addresses comment left over from PR#3841 (ampproject#3853)
  Expose submit event with on=submit:el.action syntax. (ampproject#3739)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants