EWS bubbles are being hidden due to lack of space.
Created attachment 347175 [details] Patch
Created attachment 347177 [details] Screenshot Here's a screenshot showing a typical case followed by a particularly bad case (with the proposed height increase). We could alternatively keep bumping the width up, but I figure an extra row will give us room to grow.
Comment on attachment 347175 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347175&action=review > Websites/bugs.webkit.org/ChangeLog:10 > + * template/en/default/attachment/list.html.tmpl: > + Double the height so that we have an extra row to work with. Can we just extend the width instead? I don't want every patch to occupy more vertical space.
Created attachment 347195 [details] Patch
(In reply to Ryosuke Niwa from comment #3) > Comment on attachment 347175 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=347175&action=review > > > Websites/bugs.webkit.org/ChangeLog:10 > > + * template/en/default/attachment/list.html.tmpl: > > + Double the height so that we have an extra row to work with. > > Can we just extend the width instead? I don't want every patch to occupy > more vertical space. Sure -- it seems that we need at least 710px in order to cover the "bad case" in my screenshot, though I'm sure it could get even worse (say, if the queues are backed up at a given moment and we have lots of "#10"-type numbers showing), so perhaps 750px? It doesn't look great in a tiny window, but it does get the job done for now...
Another approach is for EWS bubble page to postMessage the width of bubbles to the parent window (in JS) and then Bugzilla page to adjust the bubble iframe width accordingly.
Bug 187853 is for height:20px.
Created attachment 347216 [details] Patch
(In reply to Ryosuke Niwa from comment #6) > Another approach is for EWS bubble page to postMessage the width of bubbles > to the parent window (in JS) and then Bugzilla page to adjust the bubble > iframe width accordingly. I've attempted this by mimicking the code for the Review Patch screen, but I'm not sure how to test this locally.
(In reply to Ross Kirsling from comment #9) > (In reply to Ryosuke Niwa from comment #6) > > Another approach is for EWS bubble page to postMessage the width of bubbles > > to the parent window (in JS) and then Bugzilla page to adjust the bubble > > iframe width accordingly. > > I've attempted this by mimicking the code for the Review Patch screen, but > I'm not sure how to test this locally. Er, hmm. This is definitely not going to work, since there's no allowance for multiple EWS iframes on one page (since that's not something the Review Patch screen needs to worry about). I'm going to obsolete this patch and unobsolete the previous one for now.
How about this solution? https://stackoverflow.com/a/32162488/2691131
(In reply to Fujii Hironori from comment #11) > How about this solution? > https://stackoverflow.com/a/32162488/2691131 That's the same-origin solution, but unfortunately, the iframe domain is webkit-queues.webkit.org. :( If you open the console on this page and execute... > document.getElementsByClassName('statusBubble')[0].getElementsByTagName('iframe')[0].contentWindow.document ...you'll see this error: > SecurityError: Blocked a frame with origin "https://bugs.webkit.org" from accessing a cross-origin frame. Protocols, domains, and ports must match.
Can you do the following in both documents to relax the SOP? > document.domain = "webkit.org"; https://developer.mozilla.org/en-US/docs/Web/Security/Same-origin_policy#Changing_origin
(In reply to Fujii Hironori from comment #13) > Can you do the following in both documents to relax the SOP? > > > document.domain = "webkit.org"; > > https://developer.mozilla.org/en-US/docs/Web/Security/Same- > origin_policy#Changing_origin Interesting, I didn't know one could "promote" their origin from subdomain to domain. I think that would work, by adding a line here: https://github.com/WebKit/webkit/blob/master/Tools/QueueStatusServer/templates/statusbubble.html#L54 But I'm not sure whether Apple folks would be okay with such an approach?
Created attachment 347325 [details] Patch
Comment on attachment 347325 [details] Patch Alright, another attempt at using the containerMetrics postMessage -- this time, I'm allowing an element ID to be passed back and forth, so that a page with multiple EWS iframes can keep track of each one. I think this approach should be valid, but I'm not certain whether it's possible for me to test, so I think I will need some support on that part. 🙇♂️
Yes, we can do testing on a separate instance once the change is complete.
Comment on attachment 347325 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347325&action=review > Websites/bugs.webkit.org/template/en/default/attachment/edit.html.tmpl:42 > + $(`.statusBubble`)[0].style.height = e.data.height; Don't you need "+px"? e.data.height + "px";
JFYI, you can use evt.source to distinguish the source iframe. > iframe.contentWindow === e.source I uploaded the example. https://output.jsbin.com/fulovogoci
(In reply to Fujii Hironori from comment #18) > Comment on attachment 347325 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=347325&action=review > > > Websites/bugs.webkit.org/template/en/default/attachment/edit.html.tmpl:42 > > + $(`.statusBubble`)[0].style.height = e.data.height; > > Don't you need "+px"? > e.data.height + "px"; Hmm, I was just mimicking this: https://github.com/WebKit/webkit/blob/master/Websites/bugs.webkit.org/code-review.js#L543-L544 But yes, (as your example shows) it seems like 'px' should be necessary... > JFYI, you can use evt.source to distinguish the source iframe. > > iframe.contentWindow === e.source > > I uploaded the example. > https://output.jsbin.com/fulovogoci Awesome! That approach will certainly be simpler. :D
Created attachment 347557 [details] Patch
Created attachment 347559 [details] Patch
Comment on attachment 347559 [details] Patch Attachment 347559 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8927713 New failing tests: http/tests/security/canvas-remote-read-remote-video-redirect.html
Created attachment 347623 [details] Archive of layout-test-results from ews205 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 347559 [details] Patch I believe this patch should be good to go now.
Comment on attachment 347559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347559&action=review > Websites/bugs.webkit.org/template/en/default/attachment/edit.html.tmpl:50 > +<script> > +window.addEventListener('message', function (e) { > + if (e.origin !== 'https://webkit-queues.webkit.org' || !e.data.height) > + return; > + > + const iframe = document.querySelector('.statusBubble > iframe'); > + iframe.style.height = e.data.height + 'px'; > + iframe.style.width = e.data.width + 'px'; > +}, false); > + > +function handleStatusBubbleLoad(iframe) { > + iframe.contentWindow.postMessage('containerMetrics', 'https://webkit-queues.webkit.org'); > +} > +</script> This code effectively duplicates similar code in code-review.js. I suggest that we extract the common code into a shared JavaScript function and put it in its own JavaScript file that we include from this page and code-review.js. Maybe something like: function updateFromStatusBubbleFrame(messageEvent) { if (messageEvent.origin !== 'https://webkit-queues.webkit.org') return; if (!messageEvent.data || !messageEvent.data.height) return; var mayBeFrames = Array.prototype.slice.call(document.getElementsByClassName('statusBubble')); if (!mayBeFrames) return; const iframeTagName = 'IFRAME'; function filterFrames(element, index, array) { if (element.tagName === iframeTagName) return true; if (element.firstElementChild && element.firstElementChild.tagName === iframeTagName) return true; return false; } var frameToUpdate = null; for (const frame of mayBeFrames.filter(filterFrames)) { if (frame.contentWindow === messageEvent.source) { frameToUpdate = frame; break; } } if (!frameToUpdate) return; frameToUpdate.style.height = e.data.height + 'px'; frameToUpdate.style.width = e.data.width + 'px'; } Then in both this file and code-review.js we would register this function as the event handler for the DOM MessageEvent: window.addEventListener('message', updateFromStatusBubbleFrame, false); > Websites/bugs.webkit.org/template/en/default/attachment/edit.html.tmpl:277 > <div class="statusBubble"> > <iframe src="https://webkit-queues.webkit.org/status-bubble/[% attachment.id %]" If we can move the 'class="statusBubble"' from the <div> to the <iframe> then we can simplify the code in updateFromStatusBubbleFrame() above. > Websites/bugs.webkit.org/template/en/default/attachment/edit.html.tmpl:278 > + style="width: 600px; height: 20px; border: none;" scrolling="no" onload="handleStatusBubbleLoad(this);"> This is OK as is. The semicolon (;) is not necessary. > Websites/bugs.webkit.org/template/en/default/attachment/list.html.tmpl:53 > +window.addEventListener('message', function (e) { > + if (e.origin !== 'https://webkit-queues.webkit.org' || !e.data.height) > + return; > + > + for (const iframe of document.querySelectorAll('.statusBubble > iframe')) { > + if (iframe.contentWindow !== e.source) > + continue; > + > + iframe.style.height = e.data.height + 'px'; > + iframe.style.width = e.data.width + 'px'; > + } > +}, false); This code is similar to code in code-review.js. See my suggestion above for how we can share this code.
Created attachment 347964 [details] Patch
Created attachment 347967 [details] Patch
Comment on attachment 347967 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347967&action=review > Websites/bugs.webkit.org/js/status-bubble.js:35 > + iframe.style.width = event.data.width + 'px'; Once we set the style for the frame we can stop the iteration. The version I wrote in comment 26 did this.
(In reply to Daniel Bates from comment #29) > Comment on attachment 347967 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=347967&action=review > > > Websites/bugs.webkit.org/js/status-bubble.js:35 > > + iframe.style.width = event.data.width + 'px'; > > Once we set the style for the frame we can stop the iteration. The version I > wrote in comment 26 did this. Good point! Will add to patch for landing.
Created attachment 347969 [details] Patch for landing
Comment on attachment 347969 [details] Patch for landing Clearing flags on attachment: 347969 Committed r235262: <https://trac.webkit.org/changeset/235262>
All reviewed patches have been landed. Closing bug.
<rdar://problem/43664670>
Hm... it looks like we now don't set the border to be 0 in patch review page so bubbles show a border. We should probably also set the width to be 0, 0 initially to avoid FOC.
(In reply to Ryosuke Niwa from comment #35) > Hm... it looks like we now don't set the border to be 0 in patch review page > so bubbles show a border. We should probably also set the width to be 0, 0 > initially to avoid FOC. The cause of this seems to be that we're requesting code-review.js with a 'version' query parameter... The old file is this: https://bugs.webkit.org/code-review.js?version=48 The new file is accessible either by dropping the query parameter or by incrementing it: https://bugs.webkit.org/code-review.js https://bugs.webkit.org/code-review.js?version=49 ...though I honestly can't tell whether "version 49" is a real thing or just an out-of-bounds value. Shall I just drop the query parameter?
(In reply to Ross Kirsling from comment #36) > (In reply to Ryosuke Niwa from comment #35) > > Hm... it looks like we now don't set the border to be 0 in patch review page > > so bubbles show a border. We should probably also set the width to be 0, 0 > > initially to avoid FOC. > > The cause of this seems to be that we're requesting code-review.js with a > 'version' query parameter... > > The old file is this: > https://bugs.webkit.org/code-review.js?version=48 > > The new file is accessible either by dropping the query parameter or by > incrementing it: > https://bugs.webkit.org/code-review.js > https://bugs.webkit.org/code-review.js?version=49 > > ...though I honestly can't tell whether "version 49" is a real thing or just > an out-of-bounds value. > > Shall I just drop the query parameter? Scratch that, it's simply cached. Viewing the Review Patch screen in a private window, everything already seems to be working as desired.
*** Bug 187853 has been marked as a duplicate of this bug. ***