[webkit-reviews] review canceled: [Bug 106903] [GTK] Missing call to g_object_ref while retrieving accessible table cells : [Attachment 186392] Patch proposal plus new Layout test

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has canceled Mario Sanchez Prada
<mario at webkit.org>'s request for review:
Bug 106903: [GTK] Missing call to g_object_ref while retrieving accessible
table cells
https://bugs.webkit.org/show_bug.cgi?id=106903

Attachment 186392: Patch proposal plus new Layout test
https://bugs.webkit.org/attachment.cgi?id=186392&action=review

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
(In reply to comment #22)
> (From update of attachment 186387 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=186387&action=review
> 
> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:874

> >	 AtkObject* foundCell = atk_table_ref_at(ATK_TABLE(m_element), row,
col);
> 
> Could you use GRefPtr here?

Oh! I missed that, sorry. Will change it here and in DRT as well.

> > LayoutTests/accessibility/table-cell-for-column-and-row-crash.html:14
> > +	axCell = axTable.cellForColumnAndRow(row, column);
> > +	shouldBe("axCell.role", "'AXRole: AXCell'");
> > +	axCell = null;
> > +
> > +	// We need to force a call to the Garbage Collector here so we are
> > +	// sure that axCell will get actually destroyed after using it.
> > +	window.gc();
> 
> It's good practice to use "var" to make sure your variable is scoped to the
function instead of a property on window.

Yes, but the problem here is that I wrote all my javascript code inside
functions (e.g. test(), to be run when body.onload) and if I use "var" to
declare those variables I will obviously give them the scope of those
functions, resulting on the shouldBe() statements not working, as they expect
strings with the name of globally scoped variables.

Anyway, I agree with you it's good practice, so I'll change the test not to use
functions and just to use a plain javascript block at the end of the body, as
many other tests work (and in this case I don't think the body.onload thing is
crucial anyway). Also, I will make the most of this change to replace the call
to window.gc() with a simple call to gc(), which seems to be a safer
implementation defined in js-test-pre.js.

> > LayoutTests/accessibility/table-cell-for-column-and-row-crash.html:35
> > +	   axBody = accessibilityController.focusedElement;
> > +
> > +	   axTable = axBody.childAtIndex(0);
> > +	   shouldBe("axTable.role", "'AXRole: AXTable'");
> > +
> > +	   // Trying to reference the same cell for the table
> > +	   // multiple times shouldn't result in a crash.
> > +	   for (i = 0; i < 10; i++)
> > +	     getCellForTableAndRelease(axTable, 0, 0);
> > +  }
> 
> Same here, I'd say.

Ok. Additionally, as I moved everything to a JS block at the end of body, I
also merged the content of getCellForTableAndRelease right in the for loop.

(In reply to comment #23)
> (From update of attachment 186392 [details])
> Attachment 186392 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/16373275
> 
> New failing tests:
> accessibility/table-cell-for-column-and-row-crash.html
>
(In reply to comment #24)
> (From update of attachment 186392 [details])
> Attachment 186392 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://queues.webkit.org/results/16368319
> 
> New failing tests:
> accessibility/table-cell-for-column-and-row-crash.html

I wonder if these two failures might be related to the (mis)usage of
window.gc() instead of js-test-pre.js's gc() function. Let's see...

(In reply to comment #25)
> (From update of attachment 186392 [details])
> Attachment 186392 [details] did not pass win-ews (win):
> Output: http://queues.webkit.org/results/16377241

No idea why I get this while the EWS is green at the same time. Btw, any idea
how to check the actual output in these EWS bots? It's quite hard to just wild
guess what might be going on without any other input.