-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add DataTransferItem getAsString test #8693
Add DataTransferItem getAsString test #8693
Conversation
Build PASSEDStarted: 2017-12-15 02:45:52 View more information about this build on: |
This PR is blocked on the required "Travis CI - Pull Request" check after #14499. In order to trigger it, I will close and reopen this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nits
<meta charset="utf-8"> | ||
<title>DataTransferItem Test: getAsString()</title> | ||
<link rel="author" title="Intel" href="http://www.intel.com"> | ||
<meta name="flags" content="interact"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what this does.
<script src="/resources/testharnessreport.js"></script> | ||
|
||
<p><input id="button" type="text" value="dragcharacters" style="border:2px blue solid; width:200px; height: 100px;"/></p> | ||
<p><input id="div" type="text" style="border:2px green solid; width:200px; height: 100px;"/></p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These IDs are fairly confusing since they are not buttons or divs.
let div; | ||
|
||
setup(function() { | ||
div = document.getElementById("div"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need to be inside the setup function, right? Might as well put it outside, and just call setup with a single argument.
@domenic, many thanks for your review! I just submit a new commit to address your comments, PTAL. |
Could you rebase this on master due to the Taskcluster configuration change? Sorry :/ |
- remove meta element which name is flags - rewrite setup function - use more appropriate id
d3c3fe7
to
6ecd9b1
Compare
@domenic, CI checks are all pass, it's ready for merge. :) |
No description provided.