gmane.os.opendarwin.webkit.devel

Re: Moving WebCore/accessibility codeintoWebCore/platform/

Maciej Stachowiak — 2013-05-21T19:05:08

On May 21, 2013, at 7:34 AM, Mario Sanchez Prada <mario.prada< at >samsung.com> wrote:


I think the main problem with this change is that the current WebCore/accessibility code has lots of knowledge of the DOM and render tree. Making WebCore/platform depend on these Web-specific concepts is a layering violation - imagine how it would work if it was a top-level directory Platform that was a peer of WebCore, and built before it without access to its headers. That is logically how it should operate.

If the accessibility code could be redesigned to have a good abstraction between the parts that are tightly tied to Web-specific classes and concepts, and the part that is platform-specific, then the second part could be moved to WebCore/platform. A refactoring like that could be a good improvement, but is likely hard to do.

Regards,
Maciej

_______________________________________________
webkit-dev mailing list
webkit-dev< at >lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: Moving WebCore/accessibility code into WebCore/platform/

Darin Adler — 2013-05-21T18:17:54


Again, I think in theory this is good, if it drives people to create the abstractions we need to keep our code from being littered with too literal #ifdefs.

But instead the style checker warning is pushing at least some folks to move classes into the platform directory that do not belong there, just to avoid the warning, likely creating layering violations and misusing the platform library.

Further, I am seeing this warning often when I do refactoring patches.

I think what we did in the style checker is not a good enough way to communicate the nuance of how we do this. We need a document with a statement of principles about the platform approach to refer people to. And perhaps an approach that gives a warning rather than a style error that points to that document.

— Darin_______________________________________________
webkit-dev mailing list
webkit-dev< at >lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: Moving WebCore/accessibility code intoWebCore/platform/

Jessie Berlin — 2013-05-21T18:15:57

I rolled it out in http://trac.webkit.org/changeset/150457


On Tue, May 21, 2013 at 10:58 AM, Sam Weinig <weinig< at >apple.com> wrote:

_______________________________________________
webkit-dev mailing list
webkit-dev< at >lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: Moving WebCore/accessibility code intoWebCore/platform/

Benjamin Poulain — 2013-05-21T18:14:12


For me that rule was more about asking people to think twice and add the
proper abstraction when possible.
We do not want style checker warnings, which becomes a motivation for
creating better abstractions.

For example, recently a second implementation of some of DOMFileSystemBase
was added for blackberry (most if it is an exact copy of the common code).
With a style checker warning, the author would have had to justify this
kind of decision (and hopefully fix the code instead).

Benjamin
_______________________________________________
webkit-dev mailing list
webkit-dev< at >lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: Moving WebCore/accessibility codeinto WebCore/platform/

Sam Weinig — 2013-05-21T17:58:07

On May 21, 2013, at 10:56 AM, Darin Adler <darin< at >apple.com> wrote:


Fair enough. I'll talk to Jessie about removing it, and maybe we can revisit it in the future.

- Sam_______________________________________________
webkit-dev mailing list
webkit-dev< at >lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: Moving WebCore/accessibility codeinto WebCore/platform/

Darin Adler — 2013-05-21T17:56:54


It’s an interesting principle, but I think too far from reality right now.

And it leads to crazy things like the Clipboard in the dom directory with per-platform subclasses in the platform directory.

— Darin_______________________________________________
webkit-dev mailing list
webkit-dev< at >lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: Moving WebCore/accessibility codeinto WebCore/platform/

Sam Weinig — 2013-05-21T17:42:46

On May 21, 2013, at 8:15 AM, Darin Adler <darin< at >apple.com> wrote:


This was something we discussed during the contributors meeting.  The premise was that we should aim for having no PLATFORM() #ifdefs in non-Platform and and should instead use other macros that are more descriptive.  I like that rule, but if isn't really possible, we should probably remove the check.

Accessibility is a tricky one. The way it is currently written requires both cross platform and platform specific knowledge to co-exist.  Looking at the base class, AccessibilityObject, it seems like there is a WebCore object (AccessibilityObject) that wraps a platform specific piece (m_wrapper, which on mac is a WebAccessibilityObjectWrapper and on GTK is a AtkObject, etc.).  The way I would recommend moving forward is seeing if there is some way to abstract the platform specific piece in a C++ object that could live in platform (much like we do in other areas), and make all the WebCore code work in terms of it.

-Sam

______________________

Re: Moving WebCore/accessibility codeinto WebCore/platform/

Darin Adler — 2013-05-21T17:23:13


I can’t think of a good refined version of the rule.


Not sure that would be helpful.


Sure. Jessie and Ben, when you have a chance could one or both of you comment?

— Darin_______________________________________________
webkit-dev mailing list
webkit-dev< at >lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: Moving WebCore/accessibility codeintoWebCore/platform/

Mario Sanchez Prada — 2013-05-21T16:03:24

Hi Darin,


I see. It's true that the accessibility layer is definitely not the same
thing like other ones, such as network or graphics. It's actually more
similar to the render object layer, so I guess you are right.
 

Ok.


What about refining that rule instead of just removing it? Maybe adding a
list of directories to be excluded or something like that might work fine
too. 

On this regard, I think it would be specially interesting to hear the
opinion of the people involved in the bug that tracked this addition:
https://bugs.webkit.org/show_bug.cgi?id=115567
 

That's fine. Either removing that rule from the style checker or just
refining it to better match the situation in places such as
WebCore/accessibility will work fine here.

Thanks for the feedback,
Mario

Re: Moving WebCore/accessibility codeintoWebCore/platform/

Darin Adler — 2013-05-21T15:15:15


This is a bad idea. The platform directory has never been intended as the single place for all platform-specific code. It’s the place for platform abstractions that let us cut down on platform-specific code elsewhere. It’s not a good idea to try to put all platform-specific code into the platform directory.


There’s no problem with this.


The style checker rule is wrong and should be removed. It’s better to use a platform abstraction, but a platform #ifdef is also OK.


As you can see above, I do not agree.

Moving WebCore/accessibility code intoWebCore/platform/

Mario Sanchez Prada — 2013-05-21T14:34:28

Hi all,

Following the discussion in the contributors meeting about layering violations I was thinking about moving all the accessibility stuff inside WebCore/accessibility into a new WebCore/platform/accessibility directory.

My reasoning behind this could be summarized as this:

* Accessibility code is actually already platform dependant, as every port supporting this exposes the
accessibility hierarchy in slightly different ways (ATK flattens more the hierarchy than Mac, for instance).

* Besides the AccessibilityObject wrappers and partial platform-specific implementation files (e.g. AccessibilityObjectMac.mm) present in places like WebCore/accessibility/[atk|mac], there are other bits in WebCore/accessibility that are platform specific as well (e.g. AccessibilityRenderObject). These bits are guarded with "#if PLATFORM" macros, which would still be necessary to meet the different requirements of each port.

* The number of ports adding support for accessibility is increasing, some of them sharing code al

Re: [WIP][GTK] Memory leak detection

Brian Holt — 2013-05-21T10:25:02

Hi Rego, 

Thanks for the link to the wiki, there are some very interesting additional
valgrind options that we could use. 

The goal of this work would be to automate the process so that we could link
it in to the buildbot at some point.

I'll definitely update the wiki when the patch is accepted.

Regards
Brian   

-----Original Message-----
From: webkit-dev-bounces< at >lists.webkit.org
[mailto:webkit-dev-bounces< at >lists.webkit.org] On Behalf Of Manuel Rego
Casasnovas
Sent: 21 May 2013 08:28
To: webkit-dev< at >lists.webkit.org
Cc: Eduardo Lima Mitev
Subject: Re: [webkit-dev] [WIP][GTK] Memory leak detection

Hi Brian,

On 05/20/2013 04:48 PM, Brian Holt wrote:

My mate Edu Lima has recently added documentation about how to use valgrind
to find memory leaks in WebKitGTK+ using MiniBrowser:
https://trac.webkit.org/wiki/WebKitGTK/TrackingMemoryErrors


It looks nice, once this is ready we should update the wiki accordingly.

My 2 cents,
   Rego
_______________________________________________
webkit-dev mailing list
web

Re: Does anyone build with !ENABLE(LLINT)?

Allan Sandfeld Jensen — 2013-05-21T07:45:59

As far as I understand. LLINT is disabled for platforms that have a JIT 
implementation but no LLINT implementation, since the C-loop LLINT 
implementation assumes JIT is disabled. This at least the case for Windows on 
x64.

`Allan

Re: [WIP][GTK] Memory leak detection

Manuel Rego Casasnovas — 2013-05-21T07:27:55

Hi Brian,

On 05/20/2013 04:48 PM, Brian Holt wrote:

My mate Edu Lima has recently added documentation about how to use
valgrind to find memory leaks in WebKitGTK+ using MiniBrowser:
https://trac.webkit.org/wiki/WebKitGTK/TrackingMemoryErrors


It looks nice, once this is ready we should update the wiki accordingly.

My 2 cents,
   Rego

Re: WebKit2 and ENABLE_PLUGIN_PROCESS

Gyuyoung Kim — 2013-05-21T07:18:48

I think EFL port will be fine to set ENABLE_PLUGIN_PROCESS unconditionally,
because the feature has been enabled on EFL WK2 with Netscape-plug-in.

Gyuyoung


On Tue, May 21, 2013 at 3:38 PM, Carlos Garcia Campos
<carlosgc< at >webkit.org>wrote:

_______________________________________________
webkit-dev mailing list
webkit-dev< at >lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: WebKit2 and ENABLE_PLUGIN_PROCESS

Carlos Garcia Campos — 2013-05-21T06:38:08

El lun, 20-05-2013 a las 15:15 -0700, Anders Carlsson escribió:

I think we removed the compile option in GTK port already.



_______________________________________________
webkit-dev mailing list
webkit-dev< at >lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Use BlinkMergeCandidate to merge/back port Blinkchanges

Ryosuke Niwa — 2013-05-21T04:06:07

Hi,

We’ve added BlinkMergeCandidate keyword to Bugzilla to track any Blink
changes we might want to merge.  Please add this keyword when you’re filing
a bug or posting a patch to merge a Blink change.

- R. Niwa
_______________________________________________
webkit-dev mailing list
webkit-dev< at >lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Unprefixing Page Visibility

Simon Fraser — 2013-05-21T01:28:19

Does anyone object to supporting the Page Visibility API unprefixed?
https://bugs.webkit.org/show_bug.cgi?id=102340

Should we keep support for the prefixed version?

Simon

Does anyone build with !ENABLE(LLINT)?

Geoffrey Garen — 2013-05-20T23:37:33

I'd like to remove the #ifdef, and make the interpreter an unconditional part of JSC.

It's sometimes hard to reason about the different execution engines if you can't assume that the interpreter and its basic data structures exist.

Thanks,
Geoff

WebKit2 and ENABLE_PLUGIN_PROCESS

Anders Carlsson — 2013-05-20T22:15:43

Hello,

is anyone building WebKit2 without ENABLE_PLUGIN_PROCESS set, running Netscape plug-ins in the web process?

The reason I’m asking is that having two code paths complicates things and I’d like to remove the non-plug-in process code path.

- Anders

Re: ENABLE(PAGE_POPUP) in WebCore

Anders Carlsson — 2013-05-20T22:13:54

On May 20, 2013, at 5:00 AM, Carlos Garcia Campos <carlosgc< at >webkit.org> wrote:


That’s awesome, thanks much!

- Anders

_______________________________________________
webkit-dev mailing list
webkit-dev< at >lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev